Fix bitECS loop-animation #6183
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
takahirox
force-pushed
the
CompatibleLoopAnimation
branch
from
50176fc to
3fe1e07
Compare
takahirox
force-pushed
the
CompatibleLoopAnimation
branch
from
3fe1e07 to
055ad12
Compare
keianhzo
approved these changes
Aug 8, 2023
**Problem** loop-animation with bitECS may not work correctly if multiple loop-animation components use the same animation name to refer to glTF.animation(s). **Root issue** Mozilla Hubs loop-animation component has a problem in the spec. A loop-animation component can refer to glTF.animations with animation names but the glTF specification allows non-unique names in glTF.animations so if there are multiple glTF.animations that have the same name no one can know what glTF.animations a loop-animation using that names refers to. Refer to #6153 for details. **Solution** Preprocesses glTF content before parsing to avoid the problem by adding a glTF loader plugin that converts name based animation reference in loop-animation component to index based. **Note** The old loop-animation component handler (without bitECS) seems to assume that multiple glTF.animations that have the same name have the same animation data and loop-animation refers to the first glTF.animation of the ones having the same name. The plugin follows the assumption for the compatibility, not sure if it is the best approach. With this commit we may remove name based animation reference processing codes from the loop-animation handlers but until we are confident for the change we may keep them. **Change** - Add a glTF loader plugin to convert name based animation reference in loop-animation component to index based.
takahirox
force-pushed
the
CompatibleLoopAnimation
branch
from
055ad12 to
1601593
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
loop-animationwith bitECS may not work correctly if multipleloop-animationcomponents use the same animation name to refer toglTF.animation(s).Root issue
Mozilla Hubs
loop-animationcomponent has a problem in the spec. Aloop-animationcomponent can refer toglTF.animationswith animation names but the glTF specification allows non-unique names inglTF.animationsso if there are multipleglTF.animationsthat have the same name no one can know whatglTF.animationsaloop-animationcomponent using that names refers to.Refer to #6153 for details.
Solution
Preprocesses glTF content before parsing to avoid the problem by adding a glTF loader plugin that converts name based animation reference in
loop-animationcomponent to index based.Note
The old
loop-animationcomponent handler (without bitECS) seems to assume that multipleglTF.animationsthat have the same name have the same animation data and loop-animation refers to the firstglTF.animationof the ones having the same name. The plugin follows the assumption for the compatibility, not sure if it is the best approach.With this commit we may remove name based animation reference processing codes from the
loop-animationhandlers but until we are confident for the change we may keep them.Change
loop-animationcomponent to index based.Another approach: #6153