-
-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add interior lighting to stations #5823
base: master
Are you sure you want to change the base?
Conversation
Stations in Haber space are blood red. |
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. |
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. |
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). |
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! |
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 |
Yes, all stations use those two hubs. |
@tatjam For testing big vs. small ships, I assume you're aware of Ctrl+i to change your ship model. |
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. |
@tatjam If you're feeling done, please rebase commits if/as needed, and remove draft status on PR |
Fine! The custom light feature could be implemented later on if needed. |
Going to need some review on this @Web-eWorks or/and @fluffyfreak |
Will do - not had a chance to get to this yet but it's been on my TODO list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from a couple of minor things which could be using the Color constants I think this is great work 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
camera->PrepareLighting(this, true, true); | ||
RenderModel(renderer, camera, viewCoords, viewTransform); | ||
camera->RestoreLighting(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this during hyperjump to ensure it doesn't crash / assert (using e.g. an address sanitizer build)?
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. |
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 |
How can I get the game to try loading the binary files instead of data? |
You'll want to run the CMake Keep in mind that you'll need to bump the SGM format version to add the bounds information to the binary stream that makes up an SGM file. |
Oh, but does the game automatically load the SGM files instead of the "raw" ones? |
Nevermind, it indeed does! |
I've left a "TODO", because the existing SGM code appeared to save the tags: pioneer/src/scenegraph/BinaryConverter.cpp Line 139 in a7ce69a
but they are not really used afterwards (or it appears so at a first glance, as they should be loaded from the model itself?) |
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:
Demo: https://www.youtube.com/watch?v=D56iUkvpOac