-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Option to enable deterministic rendering #11248
Conversation
1f7b932
to
c4ace52
Compare
My understanding is that the bias applies in depth buffer increments, so a bias of 1 should always result in an overwrite regardless of distance, but I haven’t tested it. Either way making an option to stablesort is totally reasonable. The caveat regarding entity id reuse - making explicit/deliberate ordering difficult - still applies, but I guess a random stable order can still be better than a random unstable order to avoid flickering. |
c4ace52
to
2fe9fd2
Compare
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.
Seems like a reasonable compromise for users who prefer this behavior. Flickering can be much worse than z-fighting, and "fix your models" is not always an option. The branch doesn't bother me: this isn't inside the hot loop.
Some small suggestions on how to explain this feature to users, but otherwise this LGTM. If there are better solutions, this is easy to amend or extend later.
2fe9fd2
to
8abb43d
Compare
Applied suggestions, although Z-fighting might not be precisely correct here. This PR does not fix z-fighting, it only fixes flickering when Z-fighting already occurs, but Z-fighting stays. |
873f21a
to
cfc756e
Compare
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.
Good, that's much clearer. And yeah, that's a sensible distinction: we're still z-fighting, but at least it's not constantly unstable.
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.
Either this should be fixed if it's wrong, or there should be a comment to affirm that it's right. I'm assuming you chose sort_unstable
for a reason.
AFAIU this is by design. I've added plenty of comments explaining current behavior. If you have suggestions how to rephrase the comments to make it more clear yet keeping comment concise, you are welcome. |
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.
minor comment issue notwithstanding, lgtm and works as expected
cfc756e
to
fccc17c
Compare
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.
LGTM
Objective
Issue #10243: rendering multiple triangles in the same place results in flickering.
Solution
Considered these alternatives:
depth_bias
may not work, because of high number of entities, so creating a material per entity is practically not possibledepth_bias
)query.par_iter().flat_map(...).collect()
to be used incheck_visibility
system (which would solve the issue since query is deterministic), and could not figure out how to make it as cheap as current approach with thread-local collectors (QueryParIter::map_collect and similar operations #11249)So adding an option to sort entities after
check_visibility
system run.Should not be too bad, because after visibility check, only a handful entities remain.
This is probably not the only source of non-determinism in Bevy, but this is one I could find so far. At least it fixes the repro example.
Changelog
DeterministicRenderingConfig
option to enable deterministic renderingTest