Skip to content
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

LDraw Loader: Include building step count in model (assemble multiple parts per step) #24868

Merged
merged 3 commits into from
Nov 5, 2022

Conversation

sbgib
Copy link
Contributor

@sbgib sbgib commented Oct 28, 2022

Adds building steps to the LDraw Loader.

numBuildingSteps and buildingStep properties are included in the model to count and index the STEP commands in the LDraw model file. This allows 'assembling' multiple parts together in one step, as you would see in an instruction booklet. The visibility of objects could be changed this way instead of with construction steps, which seem to represent each object as a separate step.

Example for car.ldr_Packed.mpd:

userData: {
   buildingStep: 0
   category: null
   constructionStep: 0
   keywords: null
   numBuildingSteps: 8
   numConstructionSteps: 57
}

In CAD software for editing LDraw models, 'building step' appears to be the common name for this. Sources:
http://www.holly-wood.it/mlcad/basic6-en.html
http://www.melkert.net/LDCad/docs/basicEdit
https://www.leocad.org/docs/tutorial1.html

@gkjohnson gkjohnson requested a review from yomboprime October 29, 2022 17:40
@yomboprime
Copy link
Collaborator

I can review this in 3 or 4 days.

@@ -2254,6 +2260,15 @@
model.userData.numConstructionSteps = stepNumber + 1;

}
computeBuildingSteps( model ) {

// Sets userData.numBuildingSteps number in the root THREE.Group object based on the userdata.buildingStep number in THREE.Group objects.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should check if model has children, just for safety.

Copy link
Collaborator

@yomboprime yomboprime left a comment

Choose a reason for hiding this comment

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

I think we should stay with "building" or "construction" steps, but not both (it would be confusing). I was a bit simplistic when implementing construction steps, so your approach could be better.

Please modify the loader example to use building steps and see better how they work. A live link of the example would be appreciated. After that, construction steps can be removed.

Finally, the LDrawLoader docs should be updated.

@sbgib
Copy link
Contributor Author

sbgib commented Nov 5, 2022

Thanks for reviewing! That makes sense. The changes have been made and here are some live examples:

Compare the effect of the Construction step / Building step slider in each case to see the intended difference. Having the flexibility to use the steps or not would be ideal, so the Assembly by part example shows how the latter can be achieved even without them. This way, no functionality is lost.

It became clear that model.traverse is preferable to model.children where sub-models (.ldr) have also been used. So, the original approach is maintained here, except that startingBuildingStep is reset to false again after pushing to subobjects. After some testing, it seems that's what is needed to respect the steps throughout the entire model.

constructionSteps are renamed to buildingSteps here as well, though that's not strictly necessary.

@sbgib sbgib requested a review from yomboprime November 5, 2022 18:17
Copy link
Collaborator

@yomboprime yomboprime left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good.

@Mugen87 Mugen87 added this to the r147 milestone Nov 5, 2022
@Mugen87 Mugen87 merged commit 4fd7f91 into mrdoob:dev Nov 5, 2022
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