Skip to content

Support for AccuDraw shortcuts to apply and create named ACS. - #9538

Merged
bbastings merged 13 commits into
masterfrom
accudraw-named-acs-support
Jul 23, 2026
Merged

Support for AccuDraw shortcuts to apply and create named ACS.#9538
bbastings merged 13 commits into
masterfrom
accudraw-named-acs-support

Conversation

@bbastings

Copy link
Copy Markdown
Contributor

Port commented off C++ code related to named ACS elements. To be useful, getACS and writeACS need to be async. Because writeACS would require an EditTxn it only returns the props for insert/update, the shortcut tools for both get and write will be implemented in another package specific to editing.

Will be used to create similar shortcuts to these from MicroStation.
image

Port commented off C++ code related to named ACS elements.
To be useful, getACS and writeACS need to be async.
Because writeACS would require an EditTxn it only returns the
props for insert/update, the shortcut tools for both
get and write will be implemented in another package for editing.
Copilot AI review requested due to automatic review settings July 23, 2026 13:51
@bbastings
bbastings requested review from a team, aruniverse and ben-polinsky as code owners July 23, 2026 13:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds frontend plumbing for AccuDraw shortcut support around named Auxiliary Coordinate Systems (ACS), including async lookup/creation of ACS props and improved serialization of Code for RPC marshalling.

Changes:

  • Converted AccuDrawShortcuts.getACS / writeACS to async APIs returning AuxCoordSystemState | undefined, and added helpers to locate/create named ACS props by code.
  • Updated ElementState.toJSON() to serialize code as CodeProps to avoid losing the code value during serialization.
  • Adjusted AuxCoordSystemState.createNew documentation/behavior around creating an “empty” code (but currently introduces a regression called out in comments).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
core/frontend/src/tools/AccuDrawTool.ts Adds async named-ACS lookup/creation logic and updates getACS/writeACS to return AuxCoordSystemState.
core/frontend/src/EntityState.ts Ensures Code is serialized as plain JSON (CodeProps) in toJSON().
core/frontend/src/AuxCoordSys.ts Changes AuxCoordSystemState.createNew to produce an empty code and updates docs.
common/changes/@itwin/core-frontend/accudraw-named-acs-support_2026-07-23-13-45-55.json Rush change file for @itwin/core-frontend.
common/api/core-frontend.api.md Updates API report for the new async signatures and createNew param name.

Comment thread core/frontend/src/AuxCoordSys.ts
Comment thread core/frontend/src/tools/AccuDrawTool.ts Outdated
Comment thread core/frontend/src/tools/AccuDrawTool.ts Outdated
Copilot AI review requested due to automatic review settings July 23, 2026 14:19
@bbastings
bbastings requested a review from a team as a code owner July 23, 2026 14:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comment thread core/frontend/src/tools/AccuDrawTool.ts
Comment thread core/frontend/src/AuxCoordSys.ts Outdated
Comment thread full-stack-tests/core/src/frontend/standalone/ViewState.test.ts
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 23, 2026 14:30
bbastings and others added 2 commits July 23, 2026 10:31
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

core/frontend/src/tools/AccuDrawTool.ts:890

  • elements.loadProps expects a CodeProps (plain JSON). Passing a Code instance can serialize incorrectly (e.g., value getter not emitted), causing the lookup by code to fail at runtime.
      const acsProps = await vp.iModel.elements.loadProps(acsCode.toJSON());

core/frontend/src/tools/AccuDrawTool.ts:954

  • When useRotation is false, calling AccuDraw.updateAuxCoordinateSystem still publishes OrientACS, which forces AccuDraw's baseRotation/rotationMode to ACS and updates rotation. This contradicts the intent of not updating rotation and can leave rotation locked to ACS.
      if (!useRotation)
        namedAcs.setRotation(currentACS.getRotation());

      AccuDraw.updateAuxCoordinateSystem(namedAcs, vp); // Sets AccuDrawFlags.OrientACS hint to orient AccuDraw to ACS...
      currentACS = namedAcs;

Copilot AI review requested due to automatic review settings July 23, 2026 14:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

core/frontend/src/AuxCoordSys.ts:95

  • AuxCoordSystemState.createNew always instantiates AuxCoordSystemSpatialState, even when invoked via AuxCoordSystem2dState/AuxCoordSystem3dState (e.g., ViewState.createAuxCoordSystem calls those). This produces the wrong subclass for 2d views and can make the ACS invalid for the view (AuxCoordSystemState.isValidForView checks the instance type).
  public static createNew(_acsName: string, iModel: IModelConnection): AuxCoordSystemState {
    return new AuxCoordSystemSpatialState({ model: IModel.dictionaryId, code: Code.createEmpty(), classFullName: this.classFullName }, iModel);
  }

core/frontend/src/tools/AccuDrawTool.ts:954

  • getACS() calls AccuDraw.updateAuxCoordinateSystem(), which sets AccuDrawFlags.OrientACS and re-orients AccuDraw to the ACS. As a result, when useRotation is false, AccuDraw's rotation can still change, contradicting the method contract ("Optionally updates ... rotation"). Preserve and restore AccuDraw's current rotation state when useRotation is false.
      if (!useRotation)
        namedAcs.setRotation(currentACS.getRotation());

      AccuDraw.updateAuxCoordinateSystem(namedAcs, vp); // Sets AccuDrawFlags.OrientACS hint to orient AccuDraw to ACS...
      currentACS = namedAcs;

Copilot AI review requested due to automatic review settings July 23, 2026 15:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

core/frontend/src/tools/AccuDrawTool.ts:953

  • When useRotation is false and an acsName is supplied, this code still calls AccuDraw.updateAuxCoordinateSystem(...), which internally calls IModelApp.accuDraw.setContext(AccuDrawFlags.OrientACS) (see AccuDraw.ts:1596-1598). That can change AccuDraw's base rotation/axes even though the caller explicitly requested not to apply rotation. The previous implementation preserved and restored the rotation mode/axes in this case; the new one does not, so getACS(..., useRotation=false) can unexpectedly reorient AccuDraw.

      if (!useRotation)
        namedAcs.setRotation(currentACS.getRotation());

      AccuDraw.updateAuxCoordinateSystem(namedAcs, vp); // Sets AccuDrawFlags.OrientACS hint to orient AccuDraw to ACS...

@aruniverse aruniverse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The named-ACS support is useful, but there are two additional async correctness issues that should be addressed before merging:

  1. Lookup failures are currently indistinguishable from a confirmed “not found” result, allowing writeACS to take the create path after an RPC or CodeSpec failure.
  2. Both public methods retain a viewport across asynchronous work without checking whether it is still AccuDraw’s current viewport before mutating viewport and global AccuDraw state.

I also agree with the existing review threads covering the createAuxCoordSystem(name) behavior regression, the derived ACS runtime type, rotation preservation, deleted regression coverage, and the empty change description.

Please add focused tests for successful lookup, confirmed absence, lookup rejection, viewport changes during lookup, spatial/3D/2D variants, and the useOrigin/useRotation combinations.

Comment thread core/frontend/src/tools/AccuDrawTool.ts Outdated
Comment thread core/frontend/src/tools/AccuDrawTool.ts Outdated
Distinguish between acs not found and found but invalid for view.
Validate viewport/accudraw after await.
Copilot AI review requested due to automatic review settings July 23, 2026 16:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

core/frontend/src/tools/AccuDrawTool.ts:952

  • When useRotation is false, AccuDraw.updateAuxCoordinateSystem(...) sets AccuDrawFlags.OrientACS, which forces accudraw.flags.baseRotation/rotationMode to RotationMode.ACS (see core/frontend/src/AccuDraw.ts:3498-3502). The previous implementation preserved the current AccuDraw rotation state in this case; the new async version no longer does, so calling getACS(name, ..., useRotation=false) will still re-orient AccuDraw to the ACS (regression vs the intent of useRotation).
      if (!useOrigin)
        namedAcs.setOrigin(currentACS.getOrigin());

      if (!useRotation)
        namedAcs.setRotation(currentACS.getRotation());

      AccuDraw.updateAuxCoordinateSystem(namedAcs, vp); // Sets AccuDrawFlags.OrientACS hint to orient AccuDraw to ACS...
      currentACS = namedAcs;

Comment thread core/frontend/src/tools/AccuDrawTool.ts Outdated
Copilot AI review requested due to automatic review settings July 23, 2026 17:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

core/frontend/src/AuxCoordSys.ts:95

  • AuxCoordSystemState.createNew always constructs an AuxCoordSystemSpatialState, even when invoked via AuxCoordSystem2dState.createNew / AuxCoordSystem3dState.createNew (e.g., ViewState.createAuxCoordSystem calls those). This contradicts the doc comment and can yield an ACS instance that is invalid for the view type (2d views will get an is3d=true ACS). Consider constructing based on classFullName via fromProps so the subclass matches the caller.
  public static createNew(_acsName: string, iModel: IModelConnection): AuxCoordSystemState {
    return new AuxCoordSystemSpatialState({ model: IModel.dictionaryId, code: Code.createEmpty(), classFullName: this.classFullName }, iModel);
  }

Comment thread core/frontend/src/tools/AccuDrawTool.ts Outdated
Copilot AI review requested due to automatic review settings July 23, 2026 17:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

core/frontend/src/AuxCoordSys.ts:95

  • AuxCoordSystemState.createNew always instantiates AuxCoordSystemSpatialState, even when invoked via AuxCoordSystem2dState.createNew or AuxCoordSystem3dState.createNew (e.g., ViewState.ts:2522 calls AuxCoordSystem2dState.createNew). This means 2d views can end up with a 3d/spatial ACS instance, which can break 2d-specific state (origin/angle) and makes the JSDoc note about calling the appropriate subclass misleading.
  public static createNew(_acsName: string, iModel: IModelConnection): AuxCoordSystemState {
    return new AuxCoordSystemSpatialState({ model: IModel.dictionaryId, code: Code.createEmpty(), classFullName: this.classFullName }, iModel);
  }

@bbastings
bbastings enabled auto-merge (squash) July 23, 2026 18:18
Copilot AI review requested due to automatic review settings July 23, 2026 18:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

core/frontend/src/tools/AccuDrawTool.ts:954

  • AccuDraw.updateAuxCoordinateSystem always sets AccuDrawFlags.OrientACS (see AccuDraw.ts:1596-1598 / 3498-3502), which forces baseRotation + rotationMode to RotationMode.ACS. When useRotation is false, this method should not change AccuDraw's rotation mode/base rotation; currently it does, which looks like a behavior regression vs the previous save/restore logic.
      AccuDraw.updateAuxCoordinateSystem(namedAcs, vp); // Sets AccuDrawFlags.OrientACS hint to orient AccuDraw to ACS...
      currentACS = namedAcs;

Comment thread core/frontend/src/tools/AccuDrawTool.ts Outdated
Comment thread core/frontend/src/AuxCoordSys.ts
Comment thread full-stack-tests/core/src/frontend/standalone/ViewState.test.ts
Copilot AI review requested due to automatic review settings July 23, 2026 19:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

core/frontend/src/tools/AccuDrawTool.ts:955

  • AccuDraw.updateAuxCoordinateSystem() always sets AccuDrawFlags.OrientACS (see AccuDraw.ts:1578-1598), which forces accudraw.flags.baseRotation/rotationMode to RotationMode.ACS (AccuDraw.ts:3498-3502). When useRotation is false, this method should preserve the caller’s existing AccuDraw rotation mode/base rotation/axes; otherwise getACS(..., useRotation=false) still changes rotation context.
      if (!useRotation)
        namedAcs.setRotation(currentACS.getRotation());

      AccuDraw.updateAuxCoordinateSystem(namedAcs, vp); // Sets AccuDrawFlags.OrientACS hint to orient AccuDraw to ACS...
      currentACS = namedAcs;

core/frontend/src/AuxCoordSys.ts:95

  • AuxCoordSystemState.createNew always constructs an AuxCoordSystemSpatialState instance, even when invoked as AuxCoordSystem2dState.createNew / AuxCoordSystem3dState.createNew. That can produce an object whose runtime class doesn’t match props.classFullName, and serializes as 3d props (origin/yaw/pitch/roll) even if classFullName indicates 2d, which is inconsistent and can break consumers relying on the correct subclass/type.
  public static createNew(_acsName: string, iModel: IModelConnection): AuxCoordSystemState {
    return new AuxCoordSystemSpatialState({ model: IModel.dictionaryId, code: Code.createEmpty(), classFullName: this.classFullName }, iModel);
  }

@bbastings
bbastings merged commit 9590b49 into master Jul 23, 2026
22 checks passed
@bbastings
bbastings deleted the accudraw-named-acs-support branch July 23, 2026 20:19
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