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

Export only anim extension #691

Closed
wants to merge 42 commits into from

Conversation

elpie89
Copy link
Contributor

@elpie89 elpie89 commented Nov 22, 2019

so as discussed previously in #537 this is our idea for animation only export
I will write something about the specification on Monday morning.
In the meantime...you can play around with this.

# Conflicts:
#	3ds Max/Max2Babylon/Exporter/BabylonExporter.cs
#	3ds Max/Max2Babylon/Forms/ExporterForm.Designer.cs
# Conflicts:
#	SharedProjects/Babylon2GLTF/GLTFExporter.cs
# Conflicts:
#	3ds Max/Max2Babylon/Exporter/BabylonExporter.cs
# Conflicts:
#	3ds Max/Max2Babylon/Exporter/BabylonExporter.cs
#	SharedProjects/Babylon2GLTF/GLTFExporter.cs
# Conflicts:
#	3ds Max/Max2Babylon/Exporter/BabylonExporter.cs
#	SharedProjects/Babylon2GLTF/GLTFExporter.cs
add node to extension no matter animation state
split node RetargetingNodeExtension and RetargetingAnimationExtension
@Drigax
Copy link
Contributor

Drigax commented Nov 22, 2019

I'll leave this open to @noalak, should we take this change, or go with your proposed change for the animation only export extension?

@noalak
Copy link
Contributor

noalak commented Nov 25, 2019

Hi @elpie89, you guys made a really good job for the implementation.

The UI is very clean and the split between geometry and animations is much welcomed.
3dsMax_UI_elpie89

I have a question regarding the animation target ID though. Does the user has to manually setup the animation target ID to ensure uniqueness ?

@Drigax
The node name/ID uniqueness is indeed the main concern in this feature as @Selmar pointed out, since the glTF specification does not guarantee anything about uniqueness in node names.
From there we have several options:

  1. The user is responsible of node's name uniqueness.
  2. Add an ID to each node and node dependant objects (like animations channels). The ID can be setup by the user, like @elpie89 did. Or the unique gltf node ID can be a direct legacy from babylon node ID. Anyway, this could be a dedicated extension (UniqueId) that could be used by a wide range of extensions. The animation only extension would require the UniqueId extension and would do nothing else than simply flagging the scene as an animation scene.
  3. Automatically force node names to be unique. This is the current idea behind the GLTFExporter.AbstractMesh.GetUniqueNodeName method. This node name uniqueness was required for another engine (three.js? I don't remember). We could extend this to bones as well. The animation only extension would then only flag the scene as an animation scene.

I naturally leant towards 3rd solution as it is the easiest solution to implement. But based on elpie's great work you may want to support the 2nd solution as it could have a use for a wide range of extensions and is, I think, a good contribution to the glTF format.
Maybe you can ask Gary what he thinks about that ? ( I don't want to ping him in case he is too busy :) )

@noalak
Copy link
Contributor

noalak commented Nov 25, 2019

Actually, it's also possible to create a UniqueName extension for the 3rd solution.
This extension only purpose would be to flag the scene and certify the node names are unique.
Any loader implementation could then perform searches by name when reading files with such extension.

To summarize, I think that for internal (Babylon) usages the 3rd solution is the simpliest.
If what you want to achieve is to create a standard, both solutions are ok.

@elpie89
Copy link
Contributor Author

elpie89 commented Nov 25, 2019

yes It is up to the user to insert an ID, and it is up to him to guarantee the uniqueness of this one

in the object properties, there is already the possibility to add the ID

image

I will take some time asap to write an actual GLTF extension documentation as requested by Kronos Group.

I do not see a real complete solution for the ID uniqueness
As the animations exported can eventually blend with rig coming from other max scenes.
In case we stay in the same scene we could eventually warn the user if he tries to add two times the same ID

We can also probably end up with some tooling automation (like in unity humanoid system) to suggest/ auto-complete targets id

image

@elpie89
Copy link
Contributor Author

elpie89 commented Nov 25, 2019

I personally prefer the 2 solutions,.. as @noalak said, this way many 3rd party solutions can take advantage of this.
But I still do not see how we can guarantee uniqueness between max files.
Anyway, this is another problem, and should eventually be resolved on engine side.

@Selmar
Copy link
Contributor

Selmar commented Nov 25, 2019

I like the idea of a generic unique identifier as opposed to one specifically for animations. Some of my thoughts on this:

  • Identifiers should be applicable to any object, not just nodes.
    • Camera animations, other extensions...
  • The default unique identifier can be the object's name (modified to be unique).
  • Identifiers should probably be created (and made unique) when the object is created and never change (unless the user resets/changes it). There could be many problems if the identifier changed from one export to another due to a newly-created identifier collision (or even a simple name-change).
  • Identifiers of different object types may be identical (i.e. node and camera identifiers).
  • As mentioned, trying to guarantee uniqueness across scenes is of course not possible and should be dealt with when scenes are imported.

We should probably discuss this unique identifier topic (in the context of the animation retargeting extension) on the khronos glTF github, I doubt we are the first to raise the issue. I can create an issue later this week. Perhaps we can combine this with the extension proposal(s)? @elpie89

@Drigax
Copy link
Contributor

Drigax commented Nov 26, 2019

@noalak I would advocate for # 1. At the very least, I think the smoothest implementation would be where we should just worry about exporting a point cloud or animated scene as-is and keep our contract as simple as possible.

Consuming engines need to be able to take this scene and map each node to the objects that they want to animate using the included tracks, which should be possible without any additional information, right? Can we consider our node ID in the gltf as a scene-unique ID?

I'm currently unsure if we need to guarantee any uniqueness in our scene names at all, as the engine would already be responsible for importings the nodes that we describe in our animation-only gltf and uniquely identify them with their own schema on import. Similarly to our Unity Mechanim example, the end user is responsible for providing the gltfNode to engineNode mapping for animation retargeting, although in our Unity example, the engine explicitly forbids name clashing...

I'd advocate for exporting the nodes in scene as named, as regular glTF nodes without any additional data and imposing as few restrictions and assumptions as possible.

I do think we should get a better perspective on this from the larger glTF community. At the very least, if this is desperately needed, then it should be considered not officially supported in its current implementation.

Pinging @bghgary as well (but he's on vacation until next week, so I wouldn't expect an immediate reply), we have discussed animation retargeting implementation in glTF before, but this would be a crucial detail for the schema.

@Selmar
Copy link
Contributor

Selmar commented Nov 26, 2019

From a glTF perspective, I would be hesitant on forcing names to be unique to make an extension work.

Putting that aside, we have our reasons for not using node names. I'll elaborate on our pipeline:

  • Animation files will be reused across multiple models.
  • We have models that number over 1000 nodes.
  • We will have many animation files that target a small subset of a model's nodes.
  • Nodes are subject to name changes, because we use the node names for other purposes than animation retargeting.

Mirroring the unity pipeline is problematic, because:

  • Any name change would break all animation files that may have been exported for that model, making the pipeline more fragile than I want it to be. Worse, any name change must be done for all animation files targeting that node and all models that use these animation file(s). In practice, this all but removes the possibility to change names.
  • With the number of assets we have, requiring the user to provide a mapping for the retargeting is a small nightmare (unless naming conventions are strongly enforced).

In short, for animation retargeting, we could use names, if we force them to be unique. But, since it makes name changes near impossible, and we use names for gameplay purposes, I think it is not a good solution for our pipeline. @elpie89 ?

A compromise could be making the use of the unique ID optional. If engines use the names as a fallback if the unique ID is not present, this would allow for the lightweight approach you prefer, if an engine prefers it. This means both approaches would exist at the same time.

I do agree it is good to get a perspective on this from the glTF community, so we'll do that.

# Conflicts:
#	3ds Max/Max2Babylon/Forms/ExporterForm.Designer.cs
#	3ds Max/Max2Babylon/Forms/ExporterForm.cs
@elpie89
Copy link
Contributor Author

elpie89 commented Nov 28, 2019

So I raised an issue on KhronosGroup GLTF repository...we should discuss of this directly there

@Drigax
Copy link
Contributor

Drigax commented Dec 12, 2019

Cool, I'm going to close this PR due to checking in #702, but adding a toggle to support ASOBO_unique_id in the exporter would be a good solution, provided that it is not too difficult.

@Drigax Drigax closed this Dec 12, 2019
@elpie89 elpie89 deleted the export_only_anim_EXTENSION branch January 28, 2020 13:14
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.

4 participants