-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Gltf support #3785
Gltf support #3785
Conversation
Hooray Jenkins reported success with all tests good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested it in-game yet, but I trust you with the code changes 😉 Only changes I would like to request is to expand the .*
star imports to use explicit imports.
import org.terasology.rendering.assets.animation.MeshAnimation; | ||
import org.terasology.rendering.assets.animation.MeshAnimationData; | ||
import org.terasology.rendering.assets.animation.MeshAnimationImpl; | ||
import org.terasology.rendering.assets.animation.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No start imports 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No prob. I assume the IntelliJ default settings just got me there.
import org.terasology.rendering.assets.animation.MeshAnimation; | ||
import org.terasology.rendering.assets.animation.MeshAnimationData; | ||
import org.terasology.rendering.assets.animation.MeshAnimationImpl; | ||
import org.terasology.rendering.assets.animation.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No star import 😉
@@ -527,7 +527,7 @@ public String spawnPrefab(@Sender EntityRef sender, @CommandParam("prefabId") St | |||
|
|||
Optional<Prefab> prefab = Assets.getPrefab(prefabName); | |||
if (prefab.isPresent() && prefab.get().getComponent(LocationComponent.class) != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an idea how to use Optional
in a more idiomatic way? I think the logic can be expressed as follows, but I'm not sure how to end up with the status strings as before...
Assets.getPrefab(prefabName)
.filter(prefab -> prefab.getComponent(LocationComponent.class) != null)
.ifPresent(prefab -> entityManager.create(prefab.get(), spawnPos);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think I would do:
return Assets.getPrefab(prefabName).map(prefab -> {
LocationComponent loc = prefab.getComponent(LocationComponent.class);
if (loc != null) {
entityManager.create(prefab, spawnPos);
return "Done";
} else {
return "Prefab cannot be spawned (no location component)";
}
}).orElse("Unknown prefab");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work... I'm always hesitant to have side effects in map
and try to use ifPresent
(forEach
in Scala) for that purpose.
I think your proposal is better than using the combination of isPresent
and get()
(multiple times). 👍
public Vector3f getLocalDirection() { | ||
Vector3f result = Direction.FORWARD.getVector3f(); | ||
getLocalRotation().rotate(result, result); | ||
return result; | ||
} | ||
|
||
/** | ||
* @return The location of this component, relative to any parent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it location or rotation? And what is the difference between direction and rotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prexisting code, but direction would be a vector pointing the way the entity is facing, while rotation is a quaternion I imagine.
@@ -202,4 +244,6 @@ public int hashCode() { | |||
public boolean shouldReplicate(FieldMetadata<?, ?> field, boolean initial, boolean toOwner) { | |||
return initial || replicateChanges; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important new lines 😉
import org.terasology.math.geom.Matrix3f; | ||
import org.terasology.math.geom.Matrix4f; | ||
import org.terasology.math.geom.Vector3f; | ||
import org.terasology.math.geom.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No star imports 😉
import org.terasology.rendering.assets.animation.MeshAnimationBundleData; | ||
import org.terasology.rendering.assets.animation.MeshAnimationData; | ||
import org.terasology.rendering.assets.animation.MeshAnimationFrame; | ||
import org.terasology.rendering.gltf.model.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No star imports 😉
import org.terasology.assets.management.AssetManager; | ||
import org.terasology.assets.module.annotations.RegisterAssetFileFormat; | ||
import org.terasology.rendering.assets.mesh.MeshData; | ||
import org.terasology.rendering.gltf.model.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No star imports 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give others a chance to look at this, but here's an approval from my side 🙃
Added some more potential reviewers :-) @Quarternius just updated the Metal Renegades robots to a single mesh approach that should work, would be good to get somebody to try those out. See https://github.com/MetaTerasology/MetalRenegades It would also be wonderful if we could have some help adding a page on the new If we can merge this soon'ish we could probably make a GCI task to take one of Quarternius' many models that seem fitting for the game to test them out with the new format while commenting on any difficulties doing so |
Hooray Jenkins reported success with all tests good! |
@@ -245,4 +247,10 @@ public static Matrix3f calcNormalMatrix(Matrix4f mv) { | |||
result.transpose(); | |||
return result; | |||
} | |||
|
|||
public static Quat4f extractRotation(BaseMatrix4f m) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good here to add a docstring explaining the formulas. They are somewhat dense, and not obvious to the uninitiated. Is there some upstream documentation for these formulas?
} | ||
|
||
public List<Vector3f> getVertexPositions(List<Vector3f> bonePositions, List<Quat4f> boneRotations) { | ||
public List<Vector3f> getVertexPositions(List<Matrix4f> boneTransforms) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a process for auto-generating documentation? Would documenting these methods potentially be useful for new developers to become familiar with the skeletal mesh API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the documentation is autogenerated then it is likely not providing any information of value (since it would all be derived from the method name/parameter names/etc.
I'll chuck some javadoc on these methods. I'm not sure they would be used much - primarily they exist for the rendering system and I'm not sure there is much reason to otherwise interact with them, maybe custom collision checking against exact mesh position I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I just noticed they were public methods, so thought they might be used by a developer somehow.
} | ||
return results; | ||
} | ||
|
||
public List<Vector3f> getVertexNormals(List<Vector3f> bonePositions, List<Quat4f> boneRotations) { | ||
public List<Vector3f> getVertexNormals(List<Matrix4f> boneTransforms) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add docstring?
} | ||
results.add(vertexNorm); | ||
skinMat.transformVector(norm); | ||
results.add(norm); | ||
} | ||
return results; | ||
} | ||
|
||
public int getVertexCount() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add docstring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Would like to see some Javadoc docstrings for public classes and methods, so that it might be possible to auto-generate docs. That idea is not specific to this PR, though :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hooray Jenkins reported success with all tests good! |
Tested out the new model, works! Tested out old models: not so much, which is fair if we're getting a superior format anyway. But we need to convert the old ones to From memory real quick some stuff we need to convert:
See also https://trello.com/c/lZ8TVDwN/10-gtlf-pr (if able to access) |
Update: There is now a rebased version of this PR in #3808 - easier to use for testing! Edit: Update to the update! This is now rebased on its own and more fixed :-) |
ef220da
to
3adb3bb
Compare
Hooray Jenkins reported success with all tests good! |
@immortius about the floating bug discussion mentioned at artwork: there was a bug affecting the at engine/src/main/java/org/terasology/rendering/logic/SkeletonRenderer.java :
This, combined with Behaviors PR 23, should correct both the floating bug and the weird rotation observed on deer. UPDATE: just realized you applied the scale on commit 3adb3bb - I tested the latest commit locally, can confirm it's OK. leaving the original comment for reference from chat channel |
Not sure if this is a known issue, but I found that the rotation field in gltf files appears to be completely ignored. This can be replicated by going to the "Object Properties" tab in Blender and changing the rotation values, then exporting it. None of these rotations have any effect, despite being exported with the file. |
Looking through the code, it appears that rotation is used when loading bones, but not when loading meshes. This might also cause some issues with animations, although I have not tested that. |
I made a PR #3812 to add support for transformations, although it needs more testing. |
New Oreons are ready! Many thanks to @Quaternius :-) https://github.com/MetaTerasology/Oreons I think that was the bulk of the remainder, if we can just get them all tested and PRed to their module we might finally be able to merge this while retaining most the models (and the gameplay they're used in). The Light & Shadow critters are still in an odd place. We have most the red team, but in an old storage location at https://github.com/MovingBlocks/TeraMisc/tree/master/lightandshadow/redteam - need to move to a new MetaTerasology repo. Additionally I don't see the knight in there, yet I know we have the game asset to spawn it. There's also the Dominog. I think there was even an adventurer? But maybe that was just the 2D concept art by @SuperSnark. Those might be findable via forum or elsewhere, and then likewise just need a re-export to Pinging @brylie @PAndaContron @casals @Kanonbell @Naman-sopho extra in case of interest in helping with the last few (from comments here and on model PRs, along with Naman for MOO itself). Right this moment MOO may be stuck in dependency figuring-outing as we're reworking some fundamental modules, but that shouldn't be too much hassle to sort out. Now behold! The Oreons v2.0: https://cdn.discordapp.com/attachments/541688465226596362/710248508279488532/4.gif |
I am willing to help with the model PRs. What specifically should I do? |
Hey @brylie :-) That would be super awesome. Essentially it is one or two things:
|
This has been merged via #4150! Just with rebase etc so this PR doesn't show as merged, but it is! 🎉 Thank you @immortius and everybody else involved :-) Now time for continuing the cleanup with #4156 😁 |
Contains
Adds initial support for gltf mesh, skeletal mesh and animations.
How to test
spawnPrefab "engine:cubesm"
Outstanding before merging
Let me know!