-
Notifications
You must be signed in to change notification settings - Fork 3
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
MPD and DATA extension support #27
Conversation
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 quite like the direction this is taking. Things like the removal of SubFileRef
and using direct String
references looks much simpler. There's a few things however I'm not convinced about, see individual comments for details. The blockers are probably few though, like the minimum Rust version bump. Thanks!
}; | ||
gltf.nodes.push(node); | ||
|
||
// TODO: Check the part type rather than the extension. |
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.
What amount of work is it to fix that now? Extension-based guesses is a bit ugly.
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.
There's a special comment command for determining if the file is an official or unofficial "part". This should also be updated to check if each file contains geometry commands since apparently some files embed geometry inline for things like hoses and tubes. I'm not sure how trivial this would be to implement. Calling root_file.iter(...)
can create enough geometry to crash many applications, so at least some level of instancing is necessary for medium to large models.
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 think I didn't understand then what is happening here. Why do we need to check for the official-ness of the part, and how is that related with the file extension?
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.
There's more than one type that determines if it is a part. Ideally, we could just create a mesh for each ldraw file. In practice, this creates an unmanageable number of objects in the scene when importing. Stopping the recursion at the "part" level uses more memory but makes the file easier to work with. Whether the overhead matters depends on the application.
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 I understand correctly you're saying that:
- Parsing and building a mesh for each single file in a document produces too many objects for an application to handle.
- Building a single "merged" mesh for the entire document conversely creates a mesh too big to handle.
- The middle ground is to create a smaller quantity of large-ish meshes, and for that we use a heuristic here by stopping the recursion at the part level, by guessing what is a reusable part (instancing).
Did I get the reasoning correctly?
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.
Thanks for the changes. There's a couple more things I'm not sure I understand (see comments) but that's otherwise roughly in a mergeable state. Thanks!
}; | ||
gltf.nodes.push(node); | ||
|
||
// TODO: Check the part type rather than the extension. |
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 think I didn't understand then what is happening here. Why do we need to check for the official-ness of the part, and how is that related with the file extension?
The filename_char code has been completely removed since it's not actually used anywhere. The spec encourages not using special characters in filenames but doesn't require it. Applying validation beyond just checking for utf8 won't work with MPD files since the files don't need to be files on disk. These will be strings entered by the user in applications like Studio, LeoCAD, etc. |
already covered by self.cmds.iter()
I've adjusted the path normalization to only use the OS specific separator. This also seems to be the approach used by LDView. This also uses glam to simplify some of the matrix math. Let me know if there are any issues blocking this PR. |
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'd just like to understand what looks like a heuristic to balance mesh size vs. object count (?), and after that it's good to go!
}; | ||
gltf.nodes.push(node); | ||
|
||
// TODO: Check the part type rather than the extension. |
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 I understand correctly you're saying that:
- Parsing and building a mesh for each single file in a document produces too many objects for an application to handle.
- Building a single "merged" mesh for the entire document conversely creates a mesh too big to handle.
- The middle ground is to create a smaller quantity of large-ish meshes, and for that we use a heuristic here by stopping the recursion at the part level, by guessing what is a reusable part (instancing).
Did I get the reasoning correctly?
Thanks a lot for that fantastic contribution and pushing through all the comments @ScanMountGoat, much appreciated! |
This PR adds support for documents with multiple files (MPD) as well as adding the parsing for embedded data files. This adds what I would consider the "bare minimum" functionality for working with LDraw files in an application like Blender. The main functionality missing is the BFC extension and color handling.
weldr lib
parse
returns the main file for MPD files or the entire root file otherwise. This avoids loading some submodel geometry multiple times for MPD files.SourceFile
in theparse
function, the file is assumed to be an MPD file and split into multiple files. If the split file list is empty, the entire file is used to still handle dat and ldr files normally.parse
function returns aSourceFile
instead of a reference. This simplifies the implementation but uses an additional clone. The old implementation avoided lifetime issues but didn't guarantee that the reference was valid. The return type can be changed later as long as it indicates the "main model" of the file.weldr bin
path-slash
to fix some resolve errors in Linux and MacOS.