Skip to content

Cleanup RendererSceneRender::GeometryInstance#63345

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
BastiaanOlij:split_geometry_instance
Jul 27, 2022
Merged

Cleanup RendererSceneRender::GeometryInstance#63345
akien-mga merged 1 commit into
godotengine:masterfrom
BastiaanOlij:split_geometry_instance

Conversation

@BastiaanOlij

@BastiaanOlij BastiaanOlij commented Jul 23, 2022

Copy link
Copy Markdown
Contributor

Part of the render cleanup we're doing, after discussing with Reduz we're changing a number of classes in the render device implementation where we can have a base class with basic state and just override methods for the implementation.

RendererSceneRender::GeometryInstance is one such class. As the data obtained from our scene for a geometry instance is fixed, our base class can capture this data, while the implementation in subclasses defines how this data is stored on the GPU for rendering.

@BastiaanOlij BastiaanOlij added this to the 4.0 milestone Jul 23, 2022
@BastiaanOlij BastiaanOlij self-assigned this Jul 23, 2022
reduz
reduz requested changes Jul 24, 2022
View reviewed changes
Comment thread servers/rendering/renderer_scene_render.h Outdated
@BastiaanOlij BastiaanOlij force-pushed the split_geometry_instance branch 3 times, most recently from c140feb to 6539fdc Compare July 25, 2022 11:26
@BastiaanOlij BastiaanOlij marked this pull request as ready for review July 25, 2022 11:26
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner July 25, 2022 11:26
@BastiaanOlij BastiaanOlij requested a review from clayjohn July 25, 2022 11:26
@BastiaanOlij

BastiaanOlij commented Jul 25, 2022

Copy link
Copy Markdown
Contributor Author

Ok, also applied this to the pair functions. Cleaned up nicely I think.

BastiaanOlij
BastiaanOlij commented Jul 25, 2022
View reviewed changes
Comment thread servers/rendering/renderer_scene_render.h Outdated
@BastiaanOlij

BastiaanOlij commented Jul 25, 2022

Copy link
Copy Markdown
Contributor Author

Two more outstanding questions I want to add here:

  1. Should we turn GeometryInstance into a class? It makes sense to make all member variables of GeometryInstanceBase protected. We may even contemplate inheriting Object?
  2. Should we move GeometryInstance and GeometryInstanceBase into a separate renderer_geometry_instance file? Possibly rename them to RenderGeometryInstance and RenderGeometryInstanceBase ? That also makes it nicer when we eventually introduce RenderGeometryInstanceExtension so it can be implemented in a GDExtension.

@clayjohn

clayjohn commented Jul 26, 2022

Copy link
Copy Markdown
Member

Two more outstanding questions I want to add here:

  1. Should we turn GeometryInstance into a class? It makes sense to make all member variables of GeometryInstanceBase protected. We may even contemplate inheriting Object?

I think changing it to a class is fine. If not just for style/consistency. I don't know about inheriting from Object though. I'm not sure it will benefit from anything that Object would provide. We should wait and see if it will be helpful once we get to the point of overriding GeometryInstance.

  1. Should we move GeometryInstance and GeometryInstanceBase into a separate renderer_geometry_instance file? Possibly rename them to RenderGeometryInstance and RenderGeometryInstanceBase ? That also makes it nicer when we eventually introduce RenderGeometryInstanceExtension so it can be implemented in a GDExtension.

I don't have a strong preference here, but it may be nice to separate it out into its own class. But again, this could also wait until we are ready to implement RenderGeometryInstanceExtension

@BastiaanOlij

BastiaanOlij commented Jul 26, 2022

Copy link
Copy Markdown
Contributor Author

I don't have a strong preference here, but it may be nice to separate it out into its own class. But again, this could also wait until we are ready to implement RenderGeometryInstanceExtension

I'd prefer splitting it out now, especially if we decide to change it into a class. I've found that moving code into a new file really causes havoc on rebasing and increases the likelihood of code changes getting lost. Better to get it over with now.

@reduz

reduz commented Jul 26, 2022

Copy link
Copy Markdown
Member

Inheriting Object is probably too inefficient, the bindings to GDExtension will likely expose this a bit different.

@BastiaanOlij

BastiaanOlij commented Jul 26, 2022

Copy link
Copy Markdown
Contributor Author

Inheriting Object is probably too inefficient, the bindings to GDExtension will likely expose this a bit different.

That is what I worry about as well which is why I haven't made any such change yet. We do need to think about how we're going to expose the object through GDExtension then because that requires an object to be registered with ClassDB right?

@BastiaanOlij BastiaanOlij force-pushed the split_geometry_instance branch from 6539fdc to 0bd042c Compare July 27, 2022 02:32
clayjohn
clayjohn approved these changes Jul 27, 2022
View reviewed changes

@clayjohn clayjohn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me!

@akien-mga akien-mga merged commit e8309d4 into godotengine:master Jul 27, 2022
@akien-mga

akien-mga commented Jul 27, 2022

Copy link
Copy Markdown
Member

Thanks!

@BastiaanOlij BastiaanOlij deleted the split_geometry_instance branch July 27, 2022 06:30
@Geometror

Geometror commented Jul 28, 2022

Copy link
Copy Markdown
Member

It looks like this broke the GLES3 renderer (crashes immediately after opening any project). [bisected to this commit]

Crashlog:
================================================================
CrashHandlerException: Program crashed
Engine version: Godot Engine v4.0.alpha.custom_build (14d021287bced6a7f5ab9db24936bd07b4cfdfd0)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] SelfList<RasterizerSceneGLES3::GeometryInstanceGLES3>::List::add (C:\Users\hendr\Desktop\Godot\dev\main\godot\core\templates\self_list.h:49)
[1] RasterizerSceneGLES3::GeometryInstanceGLES3::_mark_dirty (C:\Users\hendr\Desktop\Godot\dev\main\godot\drivers\gles3\rasterizer_scene_gles3.cpp:124)
[2] RasterizerSceneGLES3::geometry_instance_create (C:\Users\hendr\Desktop\Godot\dev\main\godot\drivers\gles3\rasterizer_scene_gles3.cpp:58)
[3] RendererSceneCull::instance_set_base (C:\Users\hendr\Desktop\Godot\dev\main\godot\servers\rendering\renderer_scene_cull.cpp:638)
[4] RenderingServerDefault::instance_set_base (C:\Users\hendr\Desktop\Godot\dev\main\godot\servers\rendering\rendering_server_default.h:745)
[5] VisualInstance3D::set_base (C:\Users\hendr\Desktop\Godot\dev\main\godot\scene\3d\visual_instance_3d.cpp:132)
[6] SpriteBase3D::SpriteBase3D (C:\Users\hendr\Desktop\Godot\dev\main\godot\scene\3d\sprite_3d.cpp:437)
[7] AnimatedSprite3D::AnimatedSprite3D (C:\Users\hendr\Desktop\Godot\dev\main\godot\scene\3d\sprite_3d.cpp:1292)
[8] ClassDB::creator<AnimatedSprite3D> (C:\Users\hendr\Desktop\Godot\dev\main\godot\core\object\class_db.h:139)
[9] ClassDB::instantiate (C:\Users\hendr\Desktop\Godot\dev\main\godot\core\object\class_db.cpp:340)
[10] ClassDB::class_get_default_property_value (C:\Users\hendr\Desktop\Godot\dev\main\godot\core\object\class_db.cpp:1436)
[11] get_documentation_default_value (C:\Users\hendr\Desktop\Godot\dev\main\godot\editor\doc_tools.cpp:320)
[12] DocTools::generate (C:\Users\hendr\Desktop\Godot\dev\main\godot\editor\doc_tools.cpp:417)
[13] EditorHelp::generate_doc (C:\Users\hendr\Desktop\Godot\dev\main\godot\editor\editor_help.cpp:1961)
[14] EditorNode::EditorNode (C:\Users\hendr\Desktop\Godot\dev\main\godot\editor\editor_node.cpp:5944)
[15] Main::start (C:\Users\hendr\Desktop\Godot\dev\main\godot\main\main.cpp:2508)
[16] widechar_main (C:\Users\hendr\Desktop\Godot\dev\main\godot\platform\windows\godot_windows.cpp:176)
[17] _main (C:\Users\hendr\Desktop\Godot\dev\main\godot\platform\windows\godot_windows.cpp:201)
[18] main (C:\Users\hendr\Desktop\Godot\dev\main\godot\platform\windows\godot_windows.cpp:215)
[19] WinMain (C:\Users\hendr\Desktop\Godot\dev\main\godot\platform\windows\godot_windows.cpp:229)
[20] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[21] BaseThreadInitThunk
-- END OF BACKTRACE --
================================================================

@Geometror Geometror mentioned this pull request Jul 29, 2022
Closed
@BastiaanOlij BastiaanOlij mentioned this pull request Oct 19, 2022
Open
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@reduz reduz reduz approved these changes

@clayjohn clayjohn clayjohn approved these changes

Assignees

@BastiaanOlij BastiaanOlij

Labels

enhancement topic:rendering topic:3d

Projects

None yet

Milestone

4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@BastiaanOlij @clayjohn @reduz @akien-mga @Geometror