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

Update with arbitrary containers of Handles or HandleWrappers #2253

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

IamTheCarl
Copy link
Contributor

The main purpose of this PR was to make it easier to check for duplicate handles referencing the same object, however it comes with a useful side effect.

I personally needed this because my CAD program accepts a scripting language to generate models with. The number of changes a user may make is dynamic, so I need the option to provide a variable number of new faces when updating a shell. I also need to validate that the new list of faces does not contain any duplicate handles so that I can avoid a panic and produce a more meaningful error message for the user.

The key change is that ObjectSet::new and ObjectSet::replace no longer take just arrays of Handles, but instead they take any iterable object that produces objects that can be turned into HandleWrapper.

This provides two key features:

  1. You can pass an array of either Handle or HandleWrapper. Handle Wrapper makes it much easier to check if your handles are duplicates.
  2. Any iterable object, such as a vector can be used for the update. I can only know how many shells my user will produce at runtime, so this is useful to me.

With such a workflow, things like this are now easy to implement:

    let mut result = Ok(());

    solid.update_shell(
        solid.shells().only(),
        |shell, core| {
           let new_faces: Vec<HandleWrapper<Face>> =
              vec![ /* The user does whatever they need to do to generate more faces */ ];
           
           // A HashSet is probably not an ideal way to search for duplicates on this small of a scale,
           // but it's good enough for this demo.
           let new_face_set = HashSet::from(new_faces.iter().map(|face_handle| face_handle.id()));
           
           if new_face_set.len() == new_faces.len() {
               new_faces // Nothing is wrong, we are clear to continue.
           } else {
               // This is an error situation.
               result = Err(Failure::DuplicateFaces);
               
               // Because there currently isn't a way to abort an update, we'll just return a copy of our
               // original shell this results in an unmodified solid.
               [shell.clone()]
           }
        },
        core,
    );

    return result;

I think at this point it might be good to rename HandleWrapper to ObjectReference to imply that we are comparing the references and not the objects, but I'll let you make that call. Coming up with a name that reflects the purpose of that thing is... difficult.

@IamTheCarl IamTheCarl requested a review from hannobraun as a code owner March 6, 2024 00:59
Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you, @IamTheCarl! I left a few comments to complain about this or that, but overall it looks great. Merging.

crates/fj-core/src/objects/object_set.rs Show resolved Hide resolved
crates/fj-core/src/objects/object_set.rs Show resolved Hide resolved
crates/fj-core/src/objects/object_set.rs Show resolved Hide resolved
@hannobraun hannobraun merged commit 4f8fcdb into hannobraun:main Mar 6, 2024
4 checks passed
@IamTheCarl
Copy link
Contributor Author

I'll see about applying these comments tonight, but I'm glad you overall like it.

hannobraun added a commit that referenced this pull request Mar 7, 2024
Apply suggested cleanup from #2253
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.

2 participants