iTwin / itwinjs-core

Monorepo for iTwin.js Library
https://www.itwinjs.org
MIT License
620 stars 210 forks source link

Support for reset being able to choose obscured elements #7177

Closed pmconne closed 1 month ago

pmconne commented 1 month ago

When we perform a locate we find all hits within the pick aperture and order them by priority. The user can cycle through the hits using the reset (right mouse) button. Sometimes, though, a hit entirely obscures other potential hits behind it, forcing the user to, e.g., switch to wireframe or adjust the camera to locate an element they know is present. @bbastings thinks we can do better.

Add to Viewport.readPixels an optional set of Ids of elements to ignore during pick so that when cycling, the previously-processed hits will not be drawn, revealing additional hits they may have obscured. As soon as readPixels completes, the ignored elements' visibility is reset so they will draw normally again.

Viewport.readPixels already takes a long list of arguments, several of which are optional and a few others of which should be. I added an overload accepting a single ReadPixelsArgs object encapsulating these plus the new option.

I added a new OvrFlags.InvisibleDuringPick that tells the vertex shader to discard the vertex. We already have two flags - NonLocatable and Visibility - that basically do the same thing, but they can be applied outside of readPixels (e.g., via a FeatureOverrideProvider, subcategory overrides, BatchOptions, etc). The new flag is strictly for readPixels, so we can very efficiently toggle it on and off without affecting or recomputing the less transient symbology overrides.

@bbastings made changes to locate that use this new option to permit locating elements obscured by previous hits.

pmconne commented 1 month ago

@bbastings do you want to push your locate changes to this branch?

bbastings commented 1 month ago

@bbastings do you want to push your locate changes to this branch?

Yeah, can do...just need to fix up my readPixels to use the arg version now...

My changes are just to ElementLocateManager.ts...

pmconne commented 1 month ago

@nick4598 this integration test has been flaking out recently.


  1) CloudSqlite
       should LogLevel.Trace set LogMask to ALL:
     AssertError: expected _logInfo to not have been called but was called twice
    _logInfo('CloudSqlite', '19:03:09.680 | container test1 enters DELETE state', undefined) at logInfo (/home/vsts/work/1/s/core/bentley/src/Logger.ts:287:14)
    _logInfo('CloudSqlite', '19:03:09.680 | container test1 leaves DELETE state', undefined) at logInfo (/home/vsts/work/1/s/core/bentley/src/Logger.ts:287:14)
      at Object.fail (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/sinon@17.0.2/node_modules/sinon/lib/sinon/assert.js:60:27)
      at failAssertion (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/sinon@17.0.2/node_modules/sinon/lib/sinon/assert.js:203:20)
      at Object.assert.<computed> [as notCalled] (/home/vsts/work/1/s/common/temp/node_modules/.pnpm/sinon@17.0.2/node_modules/sinon/lib/sinon/assert.js:232:17)
      at Context.<anonymous> (src/integration/CloudSqlite.test.ts:228:18)
nick4598 commented 1 month ago

Thanks ill take a look at that next week

nick4598 commented 1 month ago

FYI I merged this PR which I think should help: https://github.com/iTwin/itwinjs-core/pull/7185 Let me know if this test continues to be a problem