-
Notifications
You must be signed in to change notification settings - Fork 87
#1502 Add snap to grid to sketch mode #7893
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
base: main
Are you sure you want to change the base?
#1502 Add snap to grid to sketch mode #7893
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
CodSpeed Instrumentation Performance ReportMerging #7893 will not alter performanceComparing Summary
|
Update from sync today:
|
/// The space between major grid lines, specified in the current unit | ||
#[serde(default, skip_serializing_if = "is_default")] | ||
pub major_grid_spacing: f64, | ||
/// Specifies ow many minor grid lines to have per major grid line. |
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 typo in the field description for minor_grids_per_major
. It should read "Specifies how many minor grid lines" instead of "Specifies ow many minor grid lines".
/// Specifies ow many minor grid lines to have per major grid line. | |
/// Specifies how many minor grid lines to have per major grid line. |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
if (!args.intersects.length) return | ||
const { intersectionPoint } = args | ||
const snappedPoint = intersectionPoint.twoD.clone() | ||
const snapToGrid = this.getSettings?.().modeling.snapToGrid.current |
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.
The code accesses modeling.snapToGrid.current
without first verifying that this.getSettings?.()
returned a non-null value. This could cause a runtime error if getSettings()
returns null or undefined. Consider adding a null check before accessing the nested properties:
const settings = this.getSettings?.()
const snapToGrid = settings?.modeling.snapToGrid.current
This pattern is used correctly in other parts of the code, such as in the snapToGrid()
method.
const snapToGrid = this.getSettings?.().modeling.snapToGrid.current | |
const settings = this.getSettings?.() | |
const snapToGrid = settings?.modeling.snapToGrid.current |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
||
##### minor_grids_per_major | ||
|
||
Specifies ow many minor grid lines to have per major grid line. |
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 small typo in the documentation: "Specifies ow many minor grid lines" should be "Specifies how many minor grid lines". This affects the user-facing documentation and should be corrected for clarity.
Specifies ow many minor grid lines to have per major grid line. | |
Specifies how many minor grid lines to have per major grid line. |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Fixes #1502
After discussing with mech. engineers and the frontend team, this PR adds new grid / snapping related settings.
There are two grid types: major grid is a darker grid, it has a few lighter grids inside.
These are added as both user and project level settings, but let me know if that's not your preference.
I'm thinking if the already existing "Show Scale Grid" option that currently only applies to 3D should be connected to Sketch mode but it's tricky because it's off by default, but in 2D it should be on by default
This PR implements rendering the grid and snapping to the grid according to the above settings in sketch mode (3D not affected).
If the Fixed size grid option is true, the major and minor lines are rendered in their correct sizes, except if they become too small we don't render them anymore.
If that option is turned off, we dynamically render the most appropriate 10 multiple of the original size to show something meaningful in case the grids would become too large / too small.
I changed the UnitsMenu to show the current base unit scale true to size in Sketch mode, I thought it would be useful in general and especially when the grid is shown:



In addition there are few small related/unrelated fixes about rendering, canvas resizing.

eg. previous grid was actually rendered on top of sketch segments which I assume is not on purpose so I fixed it (testing-settings needed to be updated because of that):
Remaining issues:
Feature pane collapsing bug reported by Maxthis seems to be unrelated to this branchThings I have on my radar for next (would include in this PR but it's already too big):