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

Refactor GLTF Support #142

Open
Schneegans opened this issue Apr 21, 2020 · 1 comment
Open

Refactor GLTF Support #142

Schneegans opened this issue Apr 21, 2020 · 1 comment
Labels
discussion needed Further information is requested new feature A feature request or a general improvement idea

Comments

@Schneegans
Copy link
Member

In my opinion, the current state of our GLTF support should be improved. Here are a few thoughts:

  • We could update our version of tinygltf which would make some code obsolete.
  • Environment texture handling should be decoupled from the model. In the settings there could be separate arrays, one for cubemaps and one for models.
  • I think currently no graphics memory is freed when models are deleted. This leads to memory leaks when we reload settings or plugins.
  • The code in src/cs-graphics/internal/gltfmodel.cpp could make use of some vista classes for OpenGL wrapping in order to reduce the line count significantly.
  • Support for more GTLF features (animations?) would be awesome!

What do you think?

@Schneegans Schneegans added new feature A feature request or a general improvement idea discussion needed Further information is requested labels Apr 21, 2020
@Schneegans Schneegans mentioned this issue Apr 21, 2020
11 tasks
@bernstein
Copy link
Contributor

bernstein commented Apr 21, 2020

Sounds good

  • We could update our version of tinygltf which would make some code obsolete.

we should do that

  • Environment texture handling should be decoupled from the model. In the settings there could be separate arrays, one for cubemaps and one for models.

yes, we should definitely do that!

  • The code in src/cs-graphics/internal/gltfmodel.cpp could make use of some vista classes for OpenGL wrapping in order to reduce the line count significantly.

yes, thats true

  • Support for more GTLF features (animations?) would be awesome!

Animations would be great

  • I think currently no graphics memory is freed when models are deleted. This leads to memory leaks when we reload settings or plugins.

Actually gpu resources should be freed when a GltfLoader object and all the nodes created by it are destroyed. We are using shared_ptr with a custom deleter that frees the gpu memory on destruction but there is another design issue here:

  • How to handle shared GPU resources in general:
    1. Currently shared resources like textures and bufferdata are managed by the GltfLoader. So multiple Vista nodes might share the same VBO. So as long as a GltfLoader object and/or its created nodes exist, used gpu memory is not freed
    2. Another solution would be that every created VistaGltfNode handles its own resources. So no more shared gpu data. Then removing scene graph nodes and thereby freeing associated gpu memory is simple. A drawback is of cause gpu memory fragmentation because now every node with a mesh has its own vbo.
    3. Or we add a the gpu resource handling to the GraphicsEngine.
    4. Other ideas?
  • I just looked into the GraphicsEngine and it already handles some shared resources such as hdrbuffer. I think we should go with solution 3 and use the Engine for resource handling such as the brdf textures and vbos. And maybe we can get rid of the GltfLoader class completely. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Further information is requested new feature A feature request or a general improvement idea
Projects
None yet
Development

No branches or pull requests

2 participants