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

Avoid trying to free null RIDs in FSR2 teardown #82445

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

RandomShaper
Copy link
Member

On shutdown, removes nine of these from happening:

ERROR: Attempted to free invalid ID: 0 
   at: RenderingDeviceVulkan::_free_internal (drivers\vulkan\rendering_device_vulkan.cpp:8557)
drivers\vulkan\rendering_device_vulkan.cpp:8557 - Attempted to free invalid ID: 0 

CC @DarioSamo

@RandomShaper RandomShaper requested a review from a team as a code owner September 27, 2023 14:08
@RandomShaper RandomShaper added this to the 4.2 milestone Sep 27, 2023
@DarioSamo
Copy link
Contributor

Seems good to me, I guess I figured free() works on null like how the C one does.

@RandomShaper
Copy link
Member Author

RandomShaper commented Sep 27, 2023

As far as I can tell it's a common pattern in the rendering code to check if an RID is valid (non-null) and, in more complex areas of the code, even if a non-null RID hasn't been already released somewhere else (because of cascade freeing that RD can do). Having the null check before every free is maybe silly, when free() could already have the C's free()-like semantics you mention. Food for thought and maybe digestion for improvement. 😃 (I won't get to the next step after digestion. 🤣)

@lawnjelly
Copy link
Member

We had the same thing in 3.x .. VisualServer had a safe check for invalid RIDS until #55764 (author was very keen for this but this seemed like PTSD, and I'm not sure it has helped identify any bugs 😀 ).

This has predictably caused some regressions with Dummy servers which return NULL RIDs, and just meant client code all needs this check instead. On reflection I think it was probably better for servers to silently ignore NULL RIDs, as does ::free() etc and most library funcs.

@YuriSizov YuriSizov merged commit 3c8465c into godotengine:master Sep 27, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@RandomShaper RandomShaper deleted the fix_fsr2_little_thing branch September 28, 2023 07:36
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.

5 participants