Open
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace ad-hoc Y-up vertex construction (vx, height, vy) with ct.position([vx, vy, height]) in all 4 terrain functions: - build_terrain_gltf (Tauri viewer) - build_terrain_glb (GLB export) - compute_global_normals (normal computation) - build_terrain_section_glb (per-section large map export) Add ct.reverse_indices() for CW→CCW winding reversal in all index generation paths. Update compute_global_normals to use reversed winding order for consistent normal direction. All functions now accept &CoordTransform parameter. Callers pass CoordTransform::new(ExportProfile::StandardGltf) for now — profile selection will be wired in Task 9. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace all coordinate conversion calls in building/LGO geometry export with ct.* methods from CoordTransform: - transform_position -> ct.position - transform_normal -> ct.normal - reverse_winding -> ct.reverse_indices (always applied, removed conditional) - z_up_to_y_up_vec3 -> ct.position - z_up_to_y_up_quat -> ct.quaternion Old functions kept with #[allow(dead_code)] for Task 10 cleanup. transform_normal_by_matrix kept as-is (geometric transform, not coord conversion). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Translation: express source position in Z-up (world_x, world_y, terrain_h + world_z) then convert via ct.position() instead of ad-hoc Y-up swizzle - Yaw rotation: compute quaternion around Z axis (Z-up) then convert via ct.quaternion() instead of hardcoded Y-axis quaternion - Part B (mat_local): N/A — mat_local is only used for vertex/normal transforms in Z-up space before ct.position()/ct.normal(), never as a glTF node transform Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace `y_up: bool` with `ct: Option<&CoordTransform>` across the character animation export (Task 6) and character model helper (Task 7) pipelines. This unifies all coordinate conversion behind CoordTransform, supporting both StandardGltf and UnityGltfast profiles. Changes: - animation/character.rs: 3 functions migrated (skin+nodes, animations, split animations) — quaternion, position, and matrix transforms now use ct.quaternion(), ct.position(), ct.matrix4_col_major() - character/model.rs: helper nodes (dummy points, bounding spheres) migrated to ct.matrix4_col_major() and ct.position() - character/mesh.rs: vertex positions use ct.position(), normals use ct.normal(), and index buffer gains winding reversal via ct.reverse_indices() when ct is Some (fixes missing CW->CCW conversion for character meshes) - coord_transform.rs: add matrix4_col_major() for column-major [f32;16] input/output (matches existing z_up_to_y_up_mat4 signature, avoids row-major reshaping needed by matrix4()) - character/mod.rs: public API changed from y_up:bool to ct:Option<&CoordTransform>; external callers (commands.rs) still accept y_up:bool and construct CoordTransform internally - Updated all callers: export_cli, item/model.rs, test files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace local remap_vec3 (x,y,z)->(x,z,y) with CoordTransform methods that correctly handle handedness: position() for spatial vectors, euler_angles() for rotation keyframes, normal() for direction vectors. StandardGltf profile produces (x,z,-y) instead of the old wrong (x,z,y). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add --profile <unity|standard> flag to export_cli (default: unity) - Thread ExportProfile through ExportOptions, export_map_for_unity, build_glb_from_lmo, and export_all_buildings - Tauri viewer commands continue to use StandardGltf - Tauri export commands explicitly set StandardGltf to preserve existing behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove old standalone functions that were superseded by the CoordTransform abstraction in Tasks 1-8: scene_model.rs: - transform_position, transform_normal, reverse_winding - z_up_to_y_up_vec3, z_up_to_y_up_quat (local copies) - 4 comparison tests referencing the deleted functions math/mod.rs: - z_up_to_y_up_vec3, z_up_to_y_up_quat, z_up_to_y_up_mat4 All export paths now use CoordTransform exclusively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extras JSON bypasses glTFast, so it must use a plain Y↔Z swap without profile-specific X pre-negation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- effect/export.rs: all 12 ct.position/euler_angles/normal calls changed to extras_position/extras_euler_angles (effects export as JSON, not glTF) - character/model.rs: bsphere center in extras uses extras_position - coord_transform.rs: added extras_euler_angles method extras_* methods produce final-space coordinates without glTFast pre-negation. For StandardGltf, they match the regular methods. For UnityGltfast, they skip X pre-negation since glTFast doesn't process extras/JSON data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
StandardGltf profile was applying (x, z, -y) for positions but det(B)=-1 already handles the handedness flip via the Y↔Z swap alone. The extra -y negation produced mirrored geometry in the Tauri viewer. Fixed all transform methods (position, quaternion, extras_*, euler_angles) and updated callers. Added plan doc for the fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7825c4566
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| // Reverse winding: CW (D3D) → CCW (glTF) | ||
| ct.reverse_indices(&mut indices); |
There was a problem hiding this comment.
The triangle order emitted just above (v00,v01,v10 and v10,v01,v11) is already CCW in the StandardGltf output space ([x, height, y]), but this unconditional ct.reverse_indices flips it back to CW. Because build_map_viewer_gltf/export_terrain_gltf construct CoordTransform::new(ExportProfile::StandardGltf), standard terrain exports end up with inverted face orientation and normals (inside-out lighting/culling behavior). Winding reversal here should be conditional on profiles that preserve handedness (e.g., UnityGltfast), not applied to StandardGltf.
Useful? React with 👍 / 👎.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.