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

EXT_mesh_gpu_instancing: Suggest extensionRequired usage #2405

Merged
merged 2 commits into from
Aug 2, 2024
Merged

EXT_mesh_gpu_instancing: Suggest extensionRequired usage #2405

merged 2 commits into from
Aug 2, 2024

Conversation

zeux
Copy link
Contributor

@zeux zeux commented May 23, 2024

Using this extension without specifying it as required is guaranteed to lead to significant differences in appearance between renderers that support it and renderers that don't due to how it is specified.

Fixes #2402.

Using this extension without specifying it as required is guaranteed to lead to significant differences in appearance between renderers that support it and renderers that don't due to how it is specified.
@zeux
Copy link
Contributor Author

zeux commented May 23, 2024

There’s an instancing example (SimpleInstancing) in glTF-
Sample-Assets so I don’t know if we need a second one; not sure what the policy is wrt having examples in this repository. I plan to submit a PR for SimpleInstancing if this gets merged but I didn’t realize there are examples in this repository as well…

@bghgary
Copy link
Contributor

bghgary commented May 23, 2024

@zeux Sorry, my question isn't directed at you specifically, though you are welcome to comment also of course. Maybe @lexaknyazev and others can chime in.

not sure what the policy is wrt having examples in this repository

I'm not sure either. I think this might be the only extension with sample assets inside the extension folder itself.

@lexaknyazev
Copy link
Member

lexaknyazev commented May 23, 2024

That's 100% an oversight. Samples should be in the dedicated repo.

@javagl
Copy link
Contributor

javagl commented May 24, 2024

I agree that the models should not be here.
(Moving them to glTF-Sample-Assets will require some work (just to make clear that it's not CTRL-X/V), and ... I think that only the one with the _ID should be enough...)

@zeux
Copy link
Contributor Author

zeux commented Jul 4, 2024

Please let me know if any further changes are needed on this PR (or if there are no plans to merge it).

Copy link
Member

@emackey emackey left a comment

Choose a reason for hiding this comment

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

This is a small change that looks OK to me. Looks like the PR conversation thread got lost talking about sample models, which is out of scope for this tiny fix. Let's merge it.

@lexaknyazev lexaknyazev merged commit 54a3abd into KhronosGroup:main Aug 2, 2024
1 check passed
abbaswasim pushed a commit to abbaswasim/glTF that referenced this pull request Sep 7, 2024
kwokcb added a commit that referenced this pull request Sep 17, 2024
* KHR_materials_ior: Minor typo: plural instead of singular (#2413)

* Update anisotropy extension spec (#2409)

* Clarify and simplify formula for attenuation. (#2414)

* Add TRACE prefix (#2417)

* Update clearcoat extension spec (#2415)

* Update README. (#2427)

* Add KHR_node_hoverability (#2428)

* Add KHR_node_hoverability
* Resort extension list

* EXT_mesh_gpu_instancing: Suggest extensionRequired usage (#2405)

* Add button/link to glTF Discord (#2429)

As there is a lot more traffic to the Khronos glTF Discord than there is to Slack, a button pointing users to Discord will be helpful.

---------

Co-authored-by: Andreas Atteneder <[email protected]>
Co-authored-by: Alexey Knyazev <[email protected]>
Co-authored-by: Ed Mackey <[email protected]>
Co-authored-by: Marco Hutter <[email protected]>
Co-authored-by: Khronos Group Web Services <[email protected]>
Co-authored-by: Arseny Kapoulkine <[email protected]>
Co-authored-by: James Riordon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EXT_mesh_gpu_instancing: used vs required?
5 participants