vrm1.0 support #40
vrm1.0 support #40
Conversation
Summary of ChangesHello @tatsuya-ogawa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the library by introducing full support for VRM 1.0 models. It includes a sophisticated migration mechanism that translates the newer VRM 1.0 data into the library's established VRM 0.x internal structures, ensuring seamless integration with existing rendering and animation pipelines. The accompanying example applications have been updated to showcase this new capability, providing a clear demonstration of loading and interacting with both VRM 0.x and VRM 1.0 models. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for VRM 1.0 models by adding new model assets, updating project configurations, and implementing migration logic from VRM 1.0 to VRM 0.x structures. The example applications have been updated to allow switching between VRM 0.x and VRM 1.0 models, demonstrating the new functionality. The changes also include necessary adjustments to material handling in RealityKit to ensure consistent rendering of MToon and unlit materials.
| case .alicia: return .pi | ||
| case .vrm1: return 0 |
There was a problem hiding this comment.
There's an inconsistency in the initialRotation values across different example apps. In VisionExampleApp.swift, alicia has an initialRotation of .pi and vrm1 has 0. However, in Example/Example/ViewController.swift and Example/Example/RealityKitViewController.swift, alicia has 0 and vrm1 has .pi. This discrepancy will cause models to be oriented differently depending on which example app is used, which could be confusing or lead to unexpected behavior.
| case .alicia: return .pi | |
| case .vrm1: return 0 | |
| case .alicia: return 0 | |
| case .vrm1: return .pi |
| let materialValues = expression.materialColorBinds?.map { | ||
| VRM.BlendShapeMaster.BlendShapeGroup.MaterialValueBind(materialName: "", // We need material NAME, but VRM1 bind has material INDEX. | ||
| // This is a problem. Material migration needs access to GLTF to resolve names. | ||
| // For now, we arguably can't populate this correctly without looking up GLTF. | ||
| // We will leave it empty or try to resolve later/outside? | ||
| // `MaterialValueBind` expects `materialName`. | ||
| // We can map it if we have access to GLTF. | ||
| // But this init extension only sees `vrm1`. | ||
| propertyName: $0.type.rawValue, | ||
| targetValue: $0.targetValue) | ||
| } |
There was a problem hiding this comment.
The materialName for MaterialValueBind cannot be correctly populated within this init extension because it lacks access to the gltf object, which is needed to resolve material names from their indices. This means that material-related blend shapes will not be migrated accurately from VRM 1.0 to VRM 0.x, potentially leading to incorrect visual effects.
To fix this, consider passing the gltf object to this initializer or refactoring the blend shape migration into a static function similar to migrateMaterials that has access to both vrm1 and gltf.
| radius: sphere.radius | ||
| ) | ||
| } else if let capsule = collider.shape.capsule { | ||
| // Approximate capsule as sphere (head) |
There was a problem hiding this comment.
Approximating capsule colliders as spheres might lead to less accurate collision detection for spring bones. While this might be a pragmatic choice due to API limitations, it's important to be aware that the physics simulation might not be as precise as intended by the VRM 1.0 specification.
| // VRM 0.x: bones: [Int] (indices of root bones) | ||
| // VRM 0.x assumes uniform physics parameters for the group. | ||
| // VRM 1.0 has per-joint parameters. | ||
| // Migration: Take average or first joint's parameters? |
There was a problem hiding this comment.
When migrating spring bones, the current implementation uses only the parameters of the firstJoint for the entire BoneGroup. If individual joints within a VRM 1.0 spring have varying dragForce, gravityPower, hitRadius, or stiffness values, this simplification might not accurately represent the intended physics for all joints in the group.
| let mirror = Mirror(reflecting: vrm1.humanBones) | ||
| for child in mirror.children { | ||
| guard let label = child.label, | ||
| let humanBone1 = child.value as? VRM1.Humanoid.HumanBones.HumanBone?, | ||
| let node = humanBone1?.node else { continue } |
There was a problem hiding this comment.
Using Mirror for iterating over vrm1.humanBones can be less performant and more fragile than an explicit mapping, especially if the structure of HumanBones changes in future VRM 1.0 revisions. While it works for direct property name to bone name mapping, it's generally not considered a best practice for production code due to its dynamic nature and potential for runtime errors if property names don't match expectations.
|
|
||
| private func loadVRM() { | ||
| private func setUpUI() { | ||
| let items = VRMExampleModel.allCases.map { $0.rawValue == "AliciaSolid.vrm" ? "Alicia" : "VRM 1.0" } |
There was a problem hiding this comment.
The display names for the segmented control are currently hardcoded using a ternary operator. It would be more maintainable and scalable if the VRMExampleModel enum provided a dedicated displayName property. This would centralize the naming logic and make it easier to add or change model names in the future without modifying UI code.
| let items = VRMExampleModel.allCases.map { $0.rawValue == "AliciaSolid.vrm" ? "Alicia" : "VRM 1.0" } | |
| let items = VRMExampleModel.allCases.map { $0.displayName } |
| let items = VRMExampleModel.allCases.map { $0.rawValue == "AliciaSolid.vrm" ? "Alicia" : "VRM 1.0" } | ||
| // Simplification: We could map names better, but sticking to existing UI labels |
There was a problem hiding this comment.
The display names for the segmented control are currently hardcoded using a ternary operator. It would be more maintainable and scalable if the VRMExampleModel enum provided a dedicated displayName property. This would centralize the naming logic and make it easier to add or change model names in the future without modifying UI code.
| let items = VRMExampleModel.allCases.map { $0.rawValue == "AliciaSolid.vrm" ? "Alicia" : "VRM 1.0" } | |
| // Simplification: We could map names better, but sticking to existing UI labels | |
| let items = VRMExampleModel.allCases.map { $0.displayName } |
| ForEach(AppModel.ModelName.allCases) { model in | ||
| Text(model.rawValue).tag(model) |
There was a problem hiding this comment.
The display names for the picker are currently derived directly from rawValue. It would be more user-friendly and maintainable if AppModel.ModelName provided a dedicated displayName property. This would allow for more descriptive names in the UI without relying on raw string values, and make it easier to change display names without affecting the underlying model identifiers.
| ForEach(AppModel.ModelName.allCases) { model in | |
| Text(model.rawValue).tag(model) | |
| ForEach(AppModel.ModelName.allCases) { model in | |
| Text(model.displayName).tag(model) | |
| } |
| Text("Alicia").tag(ViewModel.ModelName.alicia) | ||
| Text("VRM 1.0").tag(ViewModel.ModelName.vrm1) |
There was a problem hiding this comment.
The display names for the picker are currently hardcoded. It would be more maintainable and consistent if ViewModel.ModelName provided a dedicated displayName property. This would centralize the naming logic and make it easier to add or change model names in the future without modifying UI code.
Picker("Model", selection: $viewModel.selectedModelName) {
ForEach(ViewModel.ModelName.allCases) { model in
Text(model.displayName).tag(model)
}
}1 participant
VRM 1.0 Support & Migration Logic
Description
This PR introduces initial support for VRM 1.0 models, including a migration layer to bridge VRM 1.0 data to the existing VRM 0.x based internal architecture. It also includes a sample VRM 1.0 model and updates the example apps to demonstrate loading.
Changes
VRMKit Core
VRMMigration.swiftto map VRM 1.0 data (meta, expressions, materials) to the existing internal VRM structure.Node.swift,VRM.swift, andVRM1.swiftto accommodate VRM 1.0 specific fields.Example Apps
VisionExample,MacExample, andVRMExampleto support loading the new VRM 1.0 model (likely via a toggle or updated loader logic).Project Configuration