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

[measurement-tools] Fix Measure Distance Tool: Incorrect distance, rise, and slope calculations in sheets with vertical exaggeration #1187

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thanhbinh-d
Copy link

Issue
Since PR 837, the MeasureDistanceTool has supported scaling measurements in sheet drawings to represent real-world distances instead of sheet distances.

However, it did not account for the vertical exaggeration factor, causing incorrect calculations for:

  • Distance
  • Vertical distance (Rise)
  • Slope

Solution
To ensure accurate calculations, the start and end points are transformed into 3D world coordinates using sheetToWorldTransform before computing:

  • Direct distance (point-to-point 3D distance)
  • Horizontal distance (Run)
  • Vertical distance (Rise)
  • Slope

This fix only applies to sheet drawings with vertical exaggeration and does not impact distance calculations in 3D world views.

Demonstration
Screenshot 2025-02-12 at 4 00 50 PM

private getDistances(): { distance: number, run: number, rise: number } {
if (this.drawingMetadata?.sheetToWorldTransform) {
// calculate distances in sheet coordinates
const adjustedStartPoint = this.adjustPointWithSheetToWorldTransform(this.adjustPointForGlobalOrigin(this.startPointRef.clone()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming startPointRef is in 'drawing' coordinates, shouldn't we first adjust the point with the sheet to world, THEN adjust the point for global origin?

Copy link
Author

Choose a reason for hiding this comment

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

The transformation order follows the approach used in the LocationMeasurement tool, which has been stable and reliable (Ref: LocationMeasurement.ts#L290).

Since this method was originally implemented by Maxime, I believe he would be the best person to clarify the reasoning behind choosing this specific order.

@Maxime-Brassard, could you provide some insights on why adjustPointForGlobalOrigin is applied before sheetToWorldTransform in this tool?

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe Alex is right, I'd test it and see if results make sense.
I think the adjustment for global coordinate should be made on the 3d points.
If it's not that way in LocationMeasurement, it's probably an oversight and an coincidence that it worked so far.
I'd test it and change it across measurements if everything looks fine.

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