New GLTF API Sample Base Class & GLTF Sample#1473
New GLTF API Sample Base Class & GLTF Sample#1473HaoranXu-ARM wants to merge 8 commits intoKhronosGroup:mainfrom
Conversation
asuessenbach
left a comment
There was a problem hiding this comment.
I think, having some GLTFApiVulkanSample base class would be a good extension to the class hierarchy.
But the responsibilities between GLTFApiVulkanSample and GLTF seems to be a bit fuzzy:
- empty virtual functions introduced with
GLTFApiVulkanSample, but only called inGLTF? - elements (like
present.pipeline.pipeline) that are members inGLTFApiVulkanSample, but created and used inGLTF; and destroyed inGLTFApiVulkanSample.
Maybe more, stopped reviewing at that point.
If we want to have something like GLTFApiVulkanSample, the tasks and responsibilities of that class should be a bit cleaner.
Or maybe we could introduce some GLTF class that provides all those capabilities and could be used by a sample, without introducing another layer above ApiVulkanSample? Would probably make such a class more widely usable.
Fixed the mismatch in site of creation and destroy.
I think the goal of The expected use case will be that developers use the |
|
Not sure about this. What we're looking for is an improved glTF loader class that adds stuff like animations. Adding another sample base class (we already have two) might not be the right direction. |
Description
Add a new GLTF API Sample base class to the framework, as well as a new GLTF sample instance to demonstrate its usage.
Note
We made the following changes to the framework:
Fixes #848
The sample builds and runs successfully on Windows (NVIDIA) and Android (Mali).
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including:
If this PR contains framework changes:
batchcommand line argument to make sure all samples still work properlySample Checklist
If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist: