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 multimesh_get_buffer_rd_rid method to RenderingServer. #98788

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

Bonkahe
Copy link
Contributor

@Bonkahe Bonkahe commented Nov 3, 2024

Implements a method ( multimesh_get_buffer_rd_rid ) which can be called on the RenderingServer, which retrieves the RenderDevice Rid of the buffer, this allows direct manipulation of the buffer, without having to take it into the main thread, this lets you perform operations like copying, but for my purposes it is more important that I can now pass that Rid to a compute shader running on the global render device, letting me write to the buffer directly in the compute shader.

This has been tested on GDScript and Mono, though much more extensively on mono, I only fetched the buffer and read it on GDScript, on Mono I used a compute shader to generate foliage, with very little performance cost.

@Bonkahe Bonkahe requested a review from a team as a code owner November 3, 2024 04:45
@clayjohn clayjohn added this to the 4.4 milestone Nov 3, 2024
@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 3, 2024

Fixed styling error pending re-run of workflow review, sorry for all the extra commits, still figuring this out.

@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 3, 2024

The error blocking the merge appears to be related to that regression you mentioned in rocket chat ERROR: EditorSettings not instantiated yet. but I believe the pr #98736 has already been merged, unless you know of something I did wrong.

@Bonkahe Bonkahe requested a review from a team as a code owner November 3, 2024 23:38
@fire
Copy link
Member

fire commented Nov 4, 2024

Are you familiar with this error?

ERROR: RenderingServer.xml: Unrecognized opening tag "[RenderDevice]" in method "multimesh_get_buffer_rd_rid" description.

I think the class is called something else.

@AThousandShips
Copy link
Member

It's RenderingDevice

@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 4, 2024

Should be resolved with latest commit, I tested this one in gdscript should be good to go.

I keep breaking things lol

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 looks good to me! I have just one suggested change in the dummy renderer

@Bonkahe Bonkahe requested a review from clayjohn November 4, 2024 19:37
@AThousandShips AThousandShips changed the title Add multimesh_get_buffer_rd_rid method into RenderingServer. Add multimesh_get_buffer_rd_rid method to RenderingServer. Nov 5, 2024
@Bonkahe Bonkahe requested review from a team as code owners November 6, 2024 23:15
@clayjohn clayjohn removed request for a team November 7, 2024 00:15
@fire
Copy link
Member

fire commented Nov 7, 2024

Would you be interested in doing this for the rendering server part of MeshInstance3D? I want to use it for Opensubdiv and Delta Mush compute shader editing of rigged skeleton meshes.

https://github.com/V-Sekai/godot-subdiv

https://github.com/V-Sekai/unity-mesh-deform-direct-delta-mush

@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 7, 2024

Would you be interested in doing this for the rendering server part of MeshInstance3D? I want to use it for Opensubdiv and Delta Mush compute shader editing of rigged skeleton meshes.

https://github.com/V-Sekai/godot-subdiv

https://github.com/V-Sekai/unity-mesh-deform-direct-delta-mush

I am going to be honest, I just spent two hours trying to sort out the merge conflicts for this, it was very nerve wrecking, after this thing is merged I might go hide under a rock for a couple months.
That being said I guess I wouldn't be against it, lol.
What buffer exactly are you hoping to access?

@fire
Copy link
Member

fire commented Nov 7, 2024

I want to access enough of the ArrayMesh/Mesh to compute new meshes from nothing or at least be able to implement compute shader mesh skinning (or subdivision) for fame and profit!

@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 7, 2024

I want to access enough of the ArrayMesh/Mesh to compute new meshes from nothing or at least be able to implement compute shader mesh skinning (or subdivision) for fame and profit!

Sure sounds interesting, let me have a gander, I'll see what I dig up.

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 great!

@AThousandShips
Copy link
Member

Please squash your commits into one, see the interactive rebase for details

@Bonkahe Bonkahe marked this pull request as draft November 10, 2024 15:44
@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 10, 2024

Currently there is an issue with this branch, if you have a GPU particles3D in the scene with Transform Align set to Z-Billboard, it will throw continual errors if you update a multimesh buffer via it's Rid randomly (in my test scene it can sometimes take a couple hundred updates for the error to occur.)
Reproducable Project: testbuffer.zip

I'm going to work to see if I can resolve this.

Fixed style error.

Updated dummy mesh_storage to move from cpp to h the return of a blank Rid on _multimesh_get_buffer_rd_rid.
@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 14, 2024

I resolved the issues from @AThousandShips and squashed all the commits into one, the issue with the GPUParticles3D compute shaders being called outside of the render thread is unrelated, see this issue here: #99025
It is instead being caused by over 1k multimeshes being in the same scene as particles, as such I do not see any reason to not merge this pull request.

@Bonkahe Bonkahe marked this pull request as ready for review November 14, 2024 21:54
@Repiteo Repiteo merged commit 0a50cef into godotengine:master Nov 18, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 18, 2024

Thanks! Congratulations on your first contribution! 🎉

@Bonkahe
Copy link
Contributor Author

Bonkahe commented Nov 18, 2024

Thanks! Congratulations on your first contribution! 🎉

Woo! thank you xD

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