Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Apply codebase changes in preparation for
StandardMaterial
transmission #8704Apply codebase changes in preparation for
StandardMaterial
transmission #8704Changes from 9 commits
0a47196
eadb217
348e2ce
a12419d
77b94e4
a39364f
b5ddbd7
ac820b4
b6e564a
cec61c9
0aac272
6103ec0
e61f78d
2106a53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 change seems out of place here and wasn't mentioned in the change log.
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.
Yeah, I was also divided on wether or not to include this one. One of the systems I update in the other PR to add transmission ends up needing more than 16, but since it's technically a change unrelated to transmission I wasn't sure it would make sense to keep it there or here.
I didn't include it in the change log because it was minor and mostly internal/not user-facing, but I can also add a note about it. It's probably very obscure though, and users might be confused on why
17
, and not, say,32
(My understanding is that compile-time memory and CPU usage grows non-linearly with it, so we want to keep it low)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.
IMO that system should just use a custom (derived)
SystemParam
instead. That's the idiomatic way to handle the problem, so we should use it internally instead.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.
Interesting, I haven't used that yet; I'll try looking into it. I agree just bumping the limit arbitrarily is probably not a good solution, especially since next time when we add another feature we'll just have to bump it again.
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.
Yeah, you can also nest your parameters in tuples
SystemParam
is better, but if you don't want to bother with them, you can use tuples. Increasing this number generates a n² amount of code and slows down compile time disproportionately.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.
Ooo, I really like this tuple-based solution, if y'all are okay with me using it instead of
SystemParams
, I'd very much prefer it, sinceSystemParams
feels a bit verbose for this scenario.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.
Could the global
fog
uniform be renamed, so that in future changes to this code, we don't accidentally end up using the uniform instead of the argument?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.
I ended up renaming the argument in the fog functions instead, to
fog_params
. Right now other uniforms have a 1:1 match between their binding name and type name, with only a difference in case, (e.g.point_lights: PointLights
andcluster_offsets_and_counts: ClusterOffsetsAndCounts
) so ideally I want to preserve that consistency.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.
Maybe we want some validation here to check that the range is valid.
Also, remind me why render_range() is useful for transmission?
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.
The updated code with
.get(range)
now has validation.We use it to implement transmissive "steps", to achieve more than one layer of transparency for transmission. The items are sorted far-to-near, then split into roughly equal ranges and drawn parceled out, with a texture copy between each draw. This is kinda expensive so we default to a single step (equivalent to not using
render_range()
) but you can configure it viaCamera3d
if higher fidelity is required.Note that there are probably better ways to do this range division, including by determining which items' bounds overlap in screen space, making the steps exponentially spaced (so that near objects get more steps) or prioritizing based on roughness, transmission, and screen space coverage, however to start I just went for the most naive approach.
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.
item
isVec
here, so this should work. The difference being this skips when range is partially outside ofitems
. (sayitems.len() == 3
andrange = 2..5
)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.
Couldn't get that to work verbatim because of the nested reference in the type, so I ended up adding a
.expect()
instead, which will panic if you use an out-of-bounds range, which might also be less confusing to debug than just silently ignoring the range if it's out of bounds.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 considered using "WhiteFallbackImage" and "TransparentBlackFallbackImage"? Though I understand why you'd want to keep the current names over the ones I'm suggesting.
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.
Yeah... I tried out a bunch of different names/orders, but they all felt weird. I didn't want to rename the existing
FallbackImage
needlessly since that would introduce a breaking change for no good reason, and since there was alreadyFallbackImageCubemap
(with the “qualifier” at the end) I just went with the same pattern for consistency.I can rename all of them if there's significant demand, but since we might want to be able to (eventually) create arbitrary-color fallbacks, maybe we can save the breaking change for then?
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.
I think it's fine as-is for now