Skip to content

fix model name > 2147#31

Open
surikovniko wants to merge 4 commits intoPerseus:masterfrom
surikovniko:master
Open

fix model name > 2147#31
surikovniko wants to merge 4 commits intoPerseus:masterfrom
surikovniko:master

Conversation

@surikovniko
Copy link

If the character model name is longer than 2147, there will be problems when generating the model file name.
For example, 2148 is converted to 2148000000, which is greater than the maximum int value.

@greptile-apps
Copy link

greptile-apps bot commented Mar 2, 2026

Greptile Summary

This PR successfully fixes an integer overflow bug in LGO model file name computation by casting model_id (a u32) to u64 before multiplying by 1_000_000. The fix prevents Rust from wrapping or panicking at runtime for large model IDs.

Changes:

  • Three call sites in import_gltf_with_char_id and import_gltf_with_char_id_detailed now compute base_id as (model_id as u64) * 1_000_000u64 + mesh_idx as u64
  • The unwrap_or_else fallback in import_gltf_with_char_id is also updated with the same u64 cast

Overflow resolution:
The PR description references overflow at model_id > 2147, which relates to i32::MAX (2,147,483,647). The actual u32 arithmetic overflow occurs at model_id ≥ 4295 (since 4,295 × 1,000,000 > u32::MAX). Values in the range 2,148–4,294 do not overflow u32 in Rust but exceed i32::MAX, which is the concern for downstream consumers (e.g., game engine reading file IDs as signed integers). The fix correctly prevents both issues by moving to u64 arithmetic.

Confidence Score: 5/5

  • Safe to merge — integer overflow fix is correct, minimal, and well-scoped with no edge cases or behavioral changes.
  • The PR implements a straightforward and correct fix for the integer overflow bug. All three computation sites consistently apply the u64 cast before multiplication, preventing overflow for all practical u32 model ID values. The change is minimal, focused, and carries no risk of breaking existing functionality. No behavior change occurs for model_id < 4295 (same numeric result). The fix directly addresses the issue described in the PR without introducing complexity or side effects.
  • No files require special attention.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["import_gltf_with_char_id / import_gltf_with_char_id_detailed\nmodel_id: u32"] --> B["Iterate mesh_idx in 0..mesh_count"]
    B --> OLD["OLD: model_id * 1000000 + mesh_idx as u32\nu32 arithmetic"]
    OLD -->|"model_id < 4295"| OK1["Correct u32 result\ne.g. 725000000"]
    OLD -->|"model_id >= 4295"| ERR["u32 OVERFLOW\npanic in debug or wrap in release"]
    B --> NEW["NEW: cast model_id to u64 first\nu64 arithmetic"]
    NEW --> SAFE["Correct u64 result, no overflow"]
    SAFE --> RESULT["Reliable filename computation\ne.g. 0725000000.lgo"]
    RESULT --> WRITE["File::create ./imports/character/model/filename"]
Loading

Last reviewed commit: 264b0a5

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link

greptile-apps bot commented Mar 2, 2026

Additional Comments (3)

src-tauri/src/character/mod.rs
same overflow issue here - self.model can cause u32 overflow when > 4294

            let model_id_base = (self.model as u64) * 1_000_000u64;

src-tauri/src/character/mod.rs
need to cast these to u64 to match model_id_base type and prevent overflow

            let suit_id = (self.suit_id as u64) * 10_000u64;
            let model_id = model_id_base + suit_id + i as u64;

src-tauri/src/character/mod.rs
same overflow issue as in get_metadata - need u64 casts

            let model_id_base = (self.model as u64) * 1_000_000u64;
            let suit_id = (self.suit_id as u64) * 10_000u64;
            let model_id = model_id_base + suit_id + i as u64;

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.

1 participant