Skip to content

Systematic asset reuse for attached models #2526 #2540

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

demoncoder-crypto
Copy link

Asset Deduplication for Model Attachment

Changes:

  • Enhanced the FindDuplicateAsset method to perform thorough comparisons of different asset types:
    • For meshes: Compares file paths and buffer sizes
    • For textures: Compares file paths and dimensions
    • For height fields: Compares file paths and dimensions
    • For skins: Compares vertex and face counts
  • Updated the AppendSpec method to check for existing assets before appending new ones

This is a very rough Implementation of the issue let me know if its ok to be accepted

Copy link
Contributor

Choose a reason for hiding this comment

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

Something weird happened to this file. It looks like the code is repeated many times?

Copy link
Author

Choose a reason for hiding this comment

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

When I stashed git changes it said no changes registered added a case deleted the code tried to commit again user changes have not been stashed try merge head and after that I don't what the code file copy pasted itself. I am so sorry about this mess-up i was non intentional. I will fix this in next commit

specs_.push_back(spec);
}

// Find duplicate assets and return the existing one if found
mjSpec* mjCModel::FindDuplicateAsset(const mjSpec* spec) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing! Can you please add a test in user_api_test.cc?

Copy link
Author

Choose a reason for hiding this comment

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

For sure i will do it very soon

@yuvaltassa
Copy link
Collaborator

Comparison by metadata is not enough. You can exit early if the metadata is different, but if it's the same then you have to compare the content. Adding @kbayes who has some related infra in the asset cache.

@demoncoder-crypto
Copy link
Author

You are absolutely right as the current asset deduplication check in FindDuplicateAsset only compares metadata like file paths and dimensions. To make it more robust, you are suggesting as i understand to check if the metadata matches, then perform a byte-by-byte comparison of the actual asset content using memcmp? I can try to Implement this logic in user_model.cc. Please let me know if I am on the right track.

@demoncoder-crypto demoncoder-crypto force-pushed the asset-deduplication-feature branch from 276e320 to 3444b8e Compare March 28, 2025 11:12
@demoncoder-crypto
Copy link
Author

I have tried to implement this. I am very unsure of this. Any help is appreciated

@demoncoder-crypto
Copy link
Author

@yuvaltassa any updates on this?

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.

3 participants