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

Expose RenderSceneBuffers(RD) through ClassDB #79142

Merged
merged 1 commit into from Jul 27, 2023

Conversation

BastiaanOlij
Copy link
Contributor

In preparations of planned work around exposing some internals of the rendering engine, this PR completes the bind methods implementation of the RenderSceneBuffer and RenderSceneBufferRD objects.

Q: Should we rename RenderSceneBufferRD to RDRenderSceneBuffer to be inline with the other exposed rendering device objects?

@BastiaanOlij
Copy link
Contributor Author

I'm also wondering if we should remove if we should remove the GDVIRTUAL implementation from RenderSceneBuffers and turn that into a pure virtual class, and instead implement a RenderSceneBuffersExtension class to be used when a renderer is fully implemented in GDExtension.

@BastiaanOlij BastiaanOlij force-pushed the register_render_buffers branch 2 times, most recently from 4f3b306 to a8bbdd8 Compare July 10, 2023 11:40
@BastiaanOlij
Copy link
Contributor Author

Decided to split off RenderSceneBuffersExtension as a base class to implement RenderSceneBuffers in GDExtension. Unlikely to be used any time soon but handy to have for the future.

Also decided to keep the RenderSceneBuffersRD name.

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 10, 2023

Not sure what Schemas validity error : Element 'class', attribute 'version': The attribute 'version' is not allowed. is all about, I'm guessing this has to do with us still being in 4.1?

edit rebasing helped :)

@BastiaanOlij BastiaanOlij marked this pull request as ready for review July 10, 2023 11:44
@BastiaanOlij BastiaanOlij requested review from a team as code owners July 10, 2023 11:44
@BastiaanOlij BastiaanOlij force-pushed the register_render_buffers branch 2 times, most recently from ce29e5d to bee15d0 Compare July 10, 2023 12:33
@BastiaanOlij
Copy link
Contributor Author

After discussing with @reduz on contributors chat, decided to register RenderSceneBuffersRD as an abstract class so it can't be instantiated by users. This is always created internally in the rendering engine.

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 21, 2023

In order to make this more easily usable I've exposed get_color_texture and get_depth_texture methods that will return either the MSAA variants (when applicable) or normal variants that the extensions most like will be using. Access to underlying buffers is possible through the get_texture function. We can further extend these methods once we implement 2D HDR and in that mode potentially render directly into the render targets buffer.

In order to make this work I've moved the MSAA buffer creation back into the RenderSceneBufferRD implementation as this in hindsight makes far more sense and the code between mobile and forward+ is nearly identical.
The only difference is that while TEXTURE_USAGE_SAMPLING_BIT is used, it was omitted from the texture_is_format_supported_for_usage check in the forward+ renderer.
This needs further investigation before we merge this as this may render DATA_FORMAT_D24_UNORM_S8_UINT unusable.

I've also added access to the velocity texture.

@Zireael07
Copy link
Contributor

I've also added access to the velocity texture.

YAAAY!

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.

Great work so far! I left some comments.

doc/classes/RenderSceneBuffers.xml Outdated Show resolved Hide resolved
doc/classes/RenderSceneBuffersRD.xml Outdated Show resolved Hide resolved
reduz
reduz previously approved these changes Jul 26, 2023
doc/classes/RenderSceneBuffers.xml Outdated Show resolved Hide resolved
@reduz
Copy link
Member

reduz commented Jul 26, 2023

ah I approved by accident, it looks good but I would change this specifically.

@clayjohn clayjohn dismissed reduz’s stale review July 26, 2023 10:55

was added by mistake

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.

Looks good to me too!

@YuriSizov YuriSizov merged commit 1fe49e7 into godotengine:master Jul 27, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@BastiaanOlij BastiaanOlij deleted the register_render_buffers branch July 28, 2023 06:56
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.

None yet

6 participants