Add interior lighting to stations #5823

tatjam · 2024-04-15T07:52:36Z

Makes ships that are inside stations (as defined on the model file using tags and a series of new parser commands) have increased ambient light and reduced direct lightning, fixing the issue of black ships on eclipsed stations (Issue #4399).

TO-DO:

Working interiors
Make all stations compatible with the code
Adjustable lighting colors with some kind of station config?
Make it work with the ship cockpit and other bodies that may be found inside stations

Demo: https://www.youtube.com/watch?v=D56iUkvpOac

impaktor · 2024-04-15T13:44:15Z

Adjustable lighting colors with some kind of station config?

Stations in Haber space are blood red.

fluffyfreak · 2024-04-16T21:59:09Z

That's an interesting approach, another might be to gather the nearest 3 or 4 lights from within the station and apply them as light sources to the ship. However I'm not sure if "lights" like that are defined in the model or if that's something that could be done later.

nozmajner · 2024-04-17T05:24:30Z

There are no lights in the model. I can add them if needed. Not sure if Blender lights are exported to collada, with their settings. Or they could be tags.

tatjam · 2024-04-17T06:56:06Z

Love the idea, that way it's more in-line with the new PBR renderer idea. Guess adjusting ambient lighting is useful even then, if no dynamical env-maps are used.

Even if actual lights are used I guess the interior checking is important, to prevent outside ships from being lit. (Otherwise shadow casting is needed, or really thick exterior walls).

nozmajner · 2024-04-17T11:19:42Z

I'd suggest first doing the ambient light only, get it to merge, and then you can think about the next step, or if you want to do other features instead. No need to build a huge PR, especially when you are still getting your feet wet. This is a free-time passion project after all, avoid burnout and such!

tatjam · 2024-04-22T13:54:02Z

If I'm not mistaken all stations use either hub A o B so that should be done. Going to quickly find a "A" type station to see if it works

nozmajner · 2024-04-22T15:50:36Z

Yes, all stations use those two hubs.

impaktor · 2024-04-24T07:24:51Z

@tatjam For testing big vs. small ships, I assume you're aware of Ctrl+i to change your ship model.

tatjam · 2024-04-24T19:52:35Z

Messed around a bit with various ships and a fixed fade value feels good. There's really not any ship that's so long its center is way outside the station before it visually is outside.

impaktor · 2024-04-27T20:54:31Z

@tatjam If you're feeling done, please rebase commits if/as needed, and remove draft status on PR

tatjam · 2024-04-27T21:13:32Z

Fine! The custom light feature could be implemented later on if needed.

impaktor · 2024-05-07T14:09:50Z

Going to need some review on this @Web-eWorks or/and @fluffyfreak

Web-eWorks · 2024-05-09T01:48:54Z

Will do - not had a chance to get to this yet but it's been on my TODO list.

src/SpaceStation.cpp

src/Camera.cpp

fluffyfreak

Aside from a couple of minor things which could be using the Color constants I think this is great work 👍

Web-eWorks

This is certainly better than no lighting at all, and the implementation is fairly well-contained and easy to upgrade to shadow maps and punctual lights later down the line when we add support for them.

However, there are a few issues. The most notable one is that the new model bounds information will not be present when a model is converted to .SGM format when we ship a compiled build of the game, which will manifest as a complete lack of the new station lighting. The bounds information will need to be round-tripped through src/scenegraph/BinaryConverter.cpp in the Save and Load methods.

The second is that there is a worrying number of string comparisons and operations involved in computing interior lighting for stations. Model::DistanceFromPointToBound is called N*M times per frame, where N=number of ships on screen and M=number of stations within render distance. If a model contains more than one named bound type, there's a linear loop over bound entries by name. There is quite a bit of room for improvement here, starting with ignoring station-ship pairs with no overlap of visible cull radius. I'll leave some inline review comments where the behavior is non-optimal and presents scaling issues on lower-end hardware.

On the whole I'm quite happy with the PR and I do apologize that it took this long to review only to ask you to make major changes. I'd be happy to discuss further on the individual change requests if needed.

Web-eWorks · 2024-05-25T20:34:22Z

src/ShipCockpit.cpp

+	camera->PrepareLighting(this, true, true);
 	RenderModel(renderer, camera, viewCoords, viewTransform);
+	camera->RestoreLighting();


Have you tested this during hyperjump to ensure it doesn't crash / assert (using e.g. an address sanitizer build)?

Web-eWorks · 2024-05-25T20:59:58Z

src/scenegraph/Model.cpp

+				const auto& start = FindTagByName(bound.tags[0])->GetGlobalTransform().GetTranslate();
+				const auto& end = FindTagByName(bound.tags[1])->GetGlobalTransform().GetTranslate();


Ideally, FindTagByName shouldn't be called here at all, as it does a linear search over the list of tags present on the model (which for stations can reach the 100s). Instead, m_bounds could be defined in terms of a runtime-only struct which holds a direct pointer to the tag node, as well as a string hash for fast equality comparison with bound.for_bound. This is a lower-priority optimization if DistanceFromPointToBound is only called for ships actually within range of the station however.

Web-eWorks · 2024-05-25T21:03:27Z

src/Camera.cpp

+		SpaceStation* as_ss = (SpaceStation*)ss;
+		// Only one of these calls will do anything
+		if(as_ss->CalcInteriorLighting(b, sLight, sFac))
+			return;


This function should perform early-out rejection if the distance between the body and the space station is greater than b->GetClipRadius() + ss->GetClipRadius().

Web-eWorks · 2024-05-25T21:04:15Z

src/scenegraph/Parser.cpp

@@ -180,6 +180,14 @@ namespace SceneGraph {
 					throw ParseError("Animation start/end frames seem wrong");
 				m_model->animDefs.push_back(AnimDefinition(animName, startFrame, endFrame, loopMode));
 				return true;
+			} else if (match(token, "bound_thick_line")) {


Personally, I think bound_cylinder is a better choice of name here, but I'll leave it to your discretion whether thick_line is the technically correct name.

I've chosen "capsule" as a name instead, as this matches the typical capsule collider used in most game engines (cylinder with rounded endcaps)

Definitely a better choice - I would not have been able to tell that was the underlying shape from the existing nomenclature.

Web-eWorks · 2024-05-25T21:08:29Z

src/scenegraph/LoaderDefinitions.h

@@ -69,12 +70,36 @@ namespace SceneGraph {
 		bool loop;
 	};

+	struct BoundDefinition {


In general, I see this structure as unnecessarily future-proofed. Do you have any further plans to use tags[2] or params[1] for other bound types that you plan to implement immediately? If not, I'd recommend reducing the struct to only what's needed to express the minimum set of bounds currently in use.

I've got a feeling as most stations are going to be relatively linear, the cylinder / thick line thing is good enough. I'm going to change it to only support that, and if in the future other types are needed an union could be used.

Actually using an union doesn't seem like a great idea if strings are used to store the tags, so probably just having different meanings for the tags. For example, an AABB can also be defined by an start and end tag, and the radius could be used for rounding.

AABBs and the cylinders can get you some really complex geometries!

Web-eWorks · 2024-06-09T12:57:03Z

Just taking a first look over the code from my phone here - named struct field initializers are (AFAIK) a c++20 feature (which we don't currently target). Would recommend double-checking for that in the last few commits.

tatjam · 2024-06-09T14:54:47Z

Dangit! I've been lately working on a C++20 codebase and have gotten used to its quirks! Gonna change it back to simply setting the fields, which should be equivalent under any good compiler.

EDIT: Not using a normal aggregate initializer just because i find those to be prone to causing bugs if you reorder fields

tatjam added 6 commits April 14, 2024 23:34

Initial interior lights

3a707e8

Use tags

c58bc66

Very simple fading

b0da368

Adjust code to pioneer's code style

dd43079

Fix mishap which deleted station rings

76c987a

Adjust interiors so they include docked ships

cb34e41

Web-eWorks self-requested a review April 19, 2024 16:02

Add support for hub A

23c9726

tatjam added 4 commits April 22, 2024 16:16

Move lighting logic to SpaceStation

760fe7b

Cockpit light works, required some "structural" changes

4d26f1e

Fix transparent hub A (checked that it works)

15fbbee

Make hub A interior a bit wider so docked ships get lit

7984978

Tweak fade values

1b6bb95

tatjam marked this pull request as ready for review April 27, 2024 21:13

fluffyfreak reviewed May 10, 2024

View reviewed changes

src/SpaceStation.cpp Outdated Show resolved Hide resolved

fluffyfreak reviewed May 11, 2024

View reviewed changes

src/Camera.cpp Outdated Show resolved Hide resolved

fluffyfreak approved these changes May 11, 2024

View reviewed changes

Use color constants instead of hard-coded values

4192069

Web-eWorks requested changes May 25, 2024

View reviewed changes

tatjam added 3 commits June 9, 2024 12:58

Cleaner CalcInteriorLighting with early cull check

2b4f516

Neater bound definition, with tag caching

c599f9b

Use new bound syntax in models

6b35515

tatjam added 3 commits June 9, 2024 16:57

Do not use C++20 designated initializer

23196f7

Remove unused variable in CalcInteriorLighting

ffd956d

First attempt at binary serialized bounds

f354061

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add interior lighting to stations #5823

Add interior lighting to stations #5823

tatjam commented Apr 15, 2024 •

edited

impaktor commented Apr 15, 2024

fluffyfreak commented Apr 16, 2024

nozmajner commented Apr 17, 2024

tatjam commented Apr 17, 2024

nozmajner commented Apr 17, 2024

tatjam commented Apr 22, 2024 •

edited

nozmajner commented Apr 22, 2024

impaktor commented Apr 24, 2024

tatjam commented Apr 24, 2024

impaktor commented Apr 27, 2024

tatjam commented Apr 27, 2024

impaktor commented May 7, 2024

Web-eWorks commented May 9, 2024

fluffyfreak left a comment

Web-eWorks left a comment

Web-eWorks May 25, 2024

Web-eWorks May 25, 2024

Web-eWorks May 25, 2024 •

edited

Web-eWorks May 25, 2024

tatjam Jun 9, 2024

Web-eWorks Jun 9, 2024

Web-eWorks May 25, 2024

tatjam Jun 9, 2024

tatjam Jun 9, 2024

Web-eWorks commented Jun 9, 2024

tatjam commented Jun 9, 2024 •

edited

		const auto& start = FindTagByName(bound.tags[0])->GetGlobalTransform().GetTranslate();
		const auto& end = FindTagByName(bound.tags[1])->GetGlobalTransform().GetTranslate();

Add interior lighting to stations #5823

Are you sure you want to change the base?

Add interior lighting to stations #5823

Conversation

tatjam commented Apr 15, 2024 • edited

impaktor commented Apr 15, 2024

fluffyfreak commented Apr 16, 2024

nozmajner commented Apr 17, 2024

tatjam commented Apr 17, 2024

nozmajner commented Apr 17, 2024

tatjam commented Apr 22, 2024 • edited

nozmajner commented Apr 22, 2024

impaktor commented Apr 24, 2024

tatjam commented Apr 24, 2024

impaktor commented Apr 27, 2024

tatjam commented Apr 27, 2024

impaktor commented May 7, 2024

Web-eWorks commented May 9, 2024

fluffyfreak left a comment

Choose a reason for hiding this comment

Web-eWorks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Web-eWorks May 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Web-eWorks commented Jun 9, 2024

tatjam commented Jun 9, 2024 • edited

tatjam commented Apr 15, 2024 •

edited

tatjam commented Apr 22, 2024 •

edited

Web-eWorks May 25, 2024 •

edited

tatjam commented Jun 9, 2024 •

edited