Skip to content

WIP: Add new "is_default_object" to MakePlane #880

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
+3 −0
Closed

WIP: Add new "is_default_object" to MakePlane #880

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
294abd8
Add new "is_default_object" to MakePlane
andrewvarga May 9, 2025
a17d4da
fix doc comment syntax
andrewvarga May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions modeling-cmds/src/def_enum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,9 @@ define_modeling_cmd_enum! {
pub clobber: bool,
/// If true, the plane will be created but hidden initially.
pub hide: Option<bool>,
/// If true, the plane is made a default object. Default objects like axis planes do not get
/// included in scene bounding box calculations.
pub is_default_object: Option<bool>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be a breaking change unless we use a serde annotation to add a default value. That way when the server deserializes this from an old client that isn't sending the field, it knows whether to default such clients to true or false for this field.

What's the current behaviour? I.e. does the endpoint currently assume that any new plane made with this command is a default object, or assume it's a non-default object?

Copy link
Author

Choose a reason for hiding this comment

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

@adamchalmers thanks for looking into this. This PR has been stalled for a while due to https://github.com/KittyCAD/engine/pull/3408
I just asked on that PR how we should proceed, but to answer your question, currently every plane created is a non-default plane, which is causing the issue that zoom_to_fit is affected by the default planes being visible or hidden.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK! Then just adding a #[serde(default)] and changing the type to bool not Option<bool> should be sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, makes sense! I checked with engine and they suggested I close this approach until they figure out a better way to fix this. Here is the new issue created engine side:
https://github.com/KittyCAD/engine/issues/3589

}

/// Set the color of a plane.
Expand Down
Loading