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

Add height and width measurement tools #997

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

Conversation

saaaaaally
Copy link
Contributor

Add height and width measurement tools.
Tools are hidden by default and can be enabled with the showHeightTool and showWidthTool flags.

Height Tool:
heightTool

Width Tool:
widthTool

@aruniverse
Copy link
Member

please update the test viewer to use these new tools

Copy link
Contributor

@a-gagnon a-gagnon left a comment

Choose a reason for hiding this comment

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

I don't think it's a good idea to add logic to the DistanceMeasurement in order to support a different use-case than what was initially intended.

In my mind, height/width measurements are distance measurements, except that they're constrained to some axis when doing the initial placement. With that in mind, I'd suggest looking at the MeasurePerpendicularTool which does something very similar to what you want to achieve, but still ends up creating a DistanceMeasurement.

If that is not up to your expectations and you want different graphics for the measurement after it's placed, then you could potentially subclass the DistanceMeasurement and override a few methods. I'm thinking of setStartPoint, setEndPoint, setStartEndPoints and decorate for the most part.

@saaaaaally saaaaaally requested a review from a team as a code owner July 29, 2024 18:38
protected override writeToJSON(json: MeasurementProps) {
super.writeToJSON(json);

const jsonDist = json as PerpendicularDistanceMeasurementProps;
Copy link
Contributor

Choose a reason for hiding this comment

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

@saaaaaally Similar to the other comments, super.writeToJSON will already have written out startPoint/endPoint/showAxes to the json object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

@saaaaaally Should convert this._secondaryLine to an array of XYZProps, calling Point3d's toJSON method. While technically it's valid as written (Point3d instances can mimic XYZProps), it's still good practice to make sure the returned JSON is only primitive data + separate object instances from the original object, to allow callers to mutate it without side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

@saaaaaally Using as here doesn't do anything more than satisfy the type checker, which was already satisfied by Point3d being the same shape as XYZProps.

I was suggesting something more like

this._secondaryLine ? this._secondaryLine.map((p) => p.toJSON()) : undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@a-gagnon a-gagnon self-requested a review August 26, 2024 17:46

const title =
this._measurementType === PerpendicularMeasurementType.Height
? MeasureTools.localization.getLocalizedString("MeasureTools:tools.MeasureHeight.toolTitle").replace("{0}", fDistance)
Copy link
Contributor

Choose a reason for hiding this comment

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

@saaaaaally iTwin localization allows passing arguments when getting the localized string, e.g. instead of {0} you can have it as {{value}} and pass in a json object with { value: fDistance }. that's more robust than doing a string replace here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

@saaaaaally You don't need to do the string replace still, getLocalizedString's second argument is a TranslationOptions, and the localization library will replace {{value}} automatically when you pass in {value: fDistance}.

https://www.itwinjs.org/learning/frontend/localization/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}

public override isValidJSON(json: any): boolean {
if (!super.isValidJSON(json) || !json.hasOwnProperty("startPoint") || !json.hasOwnProperty("endPoint") || !json.hasOwnProperty("measurementType")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@saaaaaally Can omit the startPoint/endPoint check since that will be taken care of in the super call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

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.

4 participants