Skip to content
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

Fix never ending loop with overlapping probes #89134

Merged

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Mar 4, 2024

This PR changes two issues.

  1. If we have overlapping probes set to update always, we render the first probe, remove it from our probe render list, then render our second probe, where our culling sees our first probe is within our AABB, which adds the first probe back into the render list, after which we remove the second probe, and return to process the first probe, which then finds the second probe and adds it to the render list, and we keep going and going and going...
    I've changed it so the probes remain in our render lists and are only removed once we're done. That seems to fix it.
  2. After fixing that bug I ran into another bug because I had two render probes set to update always, and a 3rd render probe set to once. Godot handles the probes in reverse order so it renders the "update once" probe first. Then it renders the the "update always" probe, however as I didn't have my probe size set to 256, it tosses away the atlas and starts over. Our "update once" probe now panics as it tries to complete it's rendering process.

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Mar 4, 2024

Still looking into an issue with the second issue...

Should work now :)

@BastiaanOlij BastiaanOlij force-pushed the fix_recursive_reflection_probes branch from 4ac2253 to 30f62ee Compare March 4, 2024 09:29
@BastiaanOlij BastiaanOlij marked this pull request as ready for review March 4, 2024 09:31
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner March 4, 2024 09:31
rpi->rendering = false;
return false;
return false; // Q: this will just keep looping, consider returning true?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the other fixes we don't get here anymore, but I still think we should change this to return true; to indicate there is nothing more to render. Now if there is no atlas and we somehow get this far, we would just keep calling this endlessly for this probe with nothing really happening.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this will always continue rendering. I think that this will, in most cases, ensure that the probe gets updated the following frame (which it should). I have a feeling that setting to true will result in situations where the probe doesn't get updated at all if set to update once

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, if we return true here, it marks the probe as done, so if there wasn't another change that causes it to be re-queued for updating (like I've done in the situation where the atlas is rebuild) then it gets lost.

However if we return false, that just means it gets stuck at its current processing step and it will forever try and redo this processing step. As the failure isn't being resolved, it will never get past this point.

The only failure scenario that I can think of that would cause this situation, is when the atlas is full. If I follow the code correctly there is a condition where a probe that was rendered some frames ago will give up its spot when a new probe comes in. But it is unclear to me whether this can happen to a probe that is still being updated over multiple frames.

So if we manage to remove a probe where we completed the first processing step (e.g. rendered the faces) but where now updating on level of the mips each frame, than that probe will forever be stuck attempting to update that mip which it can never complete because it will never attempt to redo the first step.

It really seems to me that we're dealing with logic that was written once upon a time when certain things worked differently and it would retry rendering a probe in this condition, but it is definitely broken at this point in time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the comment, without an MRP this is all theory and I may have done enough to prevent ever getting into this situation.
Suggest we keep this in the back of our heads in case someone runs into a broken probe :P

@BastiaanOlij BastiaanOlij force-pushed the fix_recursive_reflection_probes branch from 30f62ee to 4cbb947 Compare March 4, 2024 09:44
@BastiaanOlij BastiaanOlij force-pushed the fix_recursive_reflection_probes branch from 4cbb947 to d04ef4a Compare March 4, 2024 10:28
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great. I replied to your comment, but I am not sure it is a good idea. I suspect that code is there for a reason and flipping the false to true will break something.

Feel free to look deeper into it if you really feel like the false should become true, but otherwise I think we can merge this once you remove the comment with the question

@BastiaanOlij BastiaanOlij force-pushed the fix_recursive_reflection_probes branch from d04ef4a to a5d3d23 Compare March 4, 2024 23:08
@akien-mga akien-mga merged commit a52e575 into godotengine:master Mar 5, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@BastiaanOlij BastiaanOlij deleted the fix_recursive_reflection_probes branch March 6, 2024 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when adding multiple reflection probes
3 participants