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

Add wireframe for compatibility mode #85621

Merged

Conversation

daustria
Copy link
Contributor

@daustria daustria commented Dec 1, 2023

The implementation for wireframe mode is mostly the same as the one in 4d50c7a
which is used in Godot 3.5

@daustria daustria requested a review from a team as a code owner December 1, 2023 18:46
@fire
Copy link
Member

fire commented Dec 1, 2023

Hi,

I noticed this is your first time contributing to Godot Engine. The Github actions which tests for acceptability to merge has found an error with your formatting.

Please see https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html for more details.

Let me know if you have questions.

@fire
Copy link
Member

fire commented Dec 1, 2023

I think this is wanted, but @clayjohn knows more about adding wireframe modes to the Godot Engine 4 compatibility renderer.

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.

This looks mostly good. Great work!

I left a few suggestions for improvements, mostly just style stuff to help this pass CI, but I left one important suggestion to clean up the implementation a bit

drivers/gles3/storage/utilities.cpp Outdated Show resolved Hide resolved
drivers/gles3/storage/mesh_storage.cpp Outdated Show resolved Hide resolved
drivers/gles3/rasterizer_scene_gles3.cpp Outdated Show resolved Hide resolved
drivers/gles3/rasterizer_scene_gles3.cpp Outdated Show resolved Hide resolved
drivers/gles3/rasterizer_scene_gles3.cpp Outdated Show resolved Hide resolved
@daustria daustria force-pushed the opengl-compatibility-wireframe branch 2 times, most recently from f9c6581 to 6f5ffcd Compare December 2, 2023 17:17
@daustria
Copy link
Contributor Author

daustria commented Dec 2, 2023

sorry about the styling issues, i think i finally figured out how to properly use clang-format
edit: seems not

@AThousandShips
Copy link
Member

You still haven't fixed the whitespace, you must not have applied clang-format correctly

@daustria daustria force-pushed the opengl-compatibility-wireframe branch from 6f5ffcd to ddeee3c Compare December 2, 2023 17:25
@AThousandShips
Copy link
Member

Your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

drivers/gles3/storage/mesh_storage.cpp Outdated Show resolved Hide resolved
drivers/gles3/storage/mesh_storage.cpp Outdated Show resolved Hide resolved
drivers/gles3/storage/mesh_storage.cpp Outdated Show resolved Hide resolved
drivers/gles3/storage/mesh_storage.cpp Outdated Show resolved Hide resolved
drivers/gles3/storage/mesh_storage.cpp Outdated Show resolved Hide resolved
drivers/gles3/storage/mesh_storage.cpp Outdated Show resolved Hide resolved
@daustria daustria force-pushed the opengl-compatibility-wireframe branch from ddeee3c to bae6f86 Compare December 2, 2023 18:10
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master d76c1d0), it mostly works as expected.

However, when using OpenGL, wireframe mode doesn't work in a running project (running with editor binary). Instead, it only works if it's toggled within the editor. This is inconsistent with Forward+/Mobile where you can change the debug draw mode to Wireframe at runtime in a debug build.

It can be useful to support this to debug procedurally generated geometry, so I'd recommend looking into fixing this issue.

func _ready():
	get_viewport().debug_draw = Viewport.DEBUG_DRAW_WIREFRAME

Testing project: test_lightmapgi_mobile.zip

Mobile Compatibility
Screenshot_20231204_170037 Screenshot_20231204_170111

@clayjohn
Copy link
Member

clayjohn commented Dec 4, 2023

However, when using OpenGL, wireframe mode doesn't work in a running project (running with editor binary). Instead, it only works if it's toggled within the editor. This is inconsistent with Forward+/Mobile where you can change the debug draw mode to Wireframe at runtime in a debug build.

It can be useful to support this to debug procedurally generated geometry, so I'd recommend looking into fixing this issue.

This is actually by design. The OpenGL renderer needs to generate a store the wireframe versions of the meshes. It would be hugely wasteful to do that at runtime.

I think that the current behaviour is preferrable (and matches 3.x) If you need the debug wireframes at runtime, you need to request them with RenderingServer.set_debug_generate_wireframes()

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! Great work

@clayjohn clayjohn added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 4, 2023
@clayjohn
Copy link
Member

clayjohn commented Dec 4, 2023

Tagged for cherrypicking. But might not be best for a 4.2.1 if we want 4.2.1 to be ultra-safe. This isn't a high priority cherrypick, but it would be nice to have in 4.2.x eventually.

@YuriSizov
Copy link
Contributor

@daustria Just FYI this commit is attributed to an email which is not linked to your account. This does not prevent us from merging it, but just be aware that you need to link your email for this to count towards your contributions to the project. See this.

@YuriSizov YuriSizov merged commit c2151bb into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks! And congrats on your first merged contribution to the engine!

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

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