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 basic multimesh data needed for headless export to the Dummy rendering server #87390

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

clayjohn
Copy link
Member

Fixes: #86396

Multimesh relies on storing and retrieving the buffer in the rendering server to function. So we need to have at least that information in the RenderingServer.

This PR adds the bare minimum amount of data to store and retrieve the multimesh buffer for export.

@clayjohn clayjohn added this to the 4.3 milestone Jan 20, 2024
@clayjohn clayjohn requested a review from a team as a code owner January 20, 2024 01:24
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

lgtm but I do think we need to think if we want to do more here. Yes this fixes the issue with data being lost while we want to keep it because we're exporting, but it is an awful lot of data to potentially keep in memory when the headless server is used to host a multiplayer game (depending on the architecture used off course).

Possibly we could add a project setting for the dummy renderer "keep mesh data" that defaults to true, and can be turned off?

@BastiaanOlij
Copy link
Contributor

Also slightly off topic, but the editor actually has a few places where we also retrieve mesh data from the rendering server. As we're getting data from the GPU that produces a stall point as we need to wait for the GPU to finish everything scheduled and wait for the data to be retrieved.

I've often wondered if it doesn't make sense to have an option to keep the data on the CPU instead of tossing it after transfering the data to the GPU. That should be an opt-in thing, maybe even on mesh level, but it could be worth thinking about.

@clayjohn
Copy link
Member Author

Also slightly off topic, but the editor actually has a few places where we also retrieve mesh data from the rendering server. As we're getting data from the GPU that produces a stall point as we need to wait for the GPU to finish everything scheduled and wait for the data to be retrieved.

I've often wondered if it doesn't make sense to have an option to keep the data on the CPU instead of tossing it after transfering the data to the GPU. That should be an opt-in thing, maybe even on mesh level, but it could be worth thinking about.

We cache it in the DummyRenderingServer right now (which is totally on the CPU)

virtual void mesh_add_surface(RID p_mesh, const RS::SurfaceData &p_surface) override {

We've been discussing with smix8 and reduz about having optional caching for when the mesh needs to be read-back regularly (I cced you on the thread)

@clayjohn
Copy link
Member Author

lgtm but I do think we need to think if we want to do more here. Yes this fixes the issue with data being lost while we want to keep it because we're exporting, but it is an awful lot of data to potentially keep in memory when the headless server is used to host a multiplayer game (depending on the architecture used off course).

Possibly we could add a project setting for the dummy renderer "keep mesh data" that defaults to true, and can be turned off?

That sounds like a good idea to explore in general for the dummy renderer as mesh data is going to be the biggest culprit by far and it is currently always cached as well

@YuriSizov YuriSizov merged commit 1018706 into godotengine:master Jan 25, 2024
16 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov
Copy link
Contributor

That sounds like a good idea to explore in general for the dummy renderer as mesh data is going to be the biggest culprit by far and it is currently always cached as well

I interpreted this as "let's merge this for now, but look into the suggestion in future". 🙃

@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 16, 2024
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 16, 2024
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.

MultiMesh buffers are sometimes reset to 0 on export
4 participants