iTwin / itwinjs-core

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

Allow context reality model visibility to be toggled on/off #7170

Closed mdastous-bentley closed 5 days ago

mdastous-bentley commented 1 week ago

Similarly to map-layers, we want to be a able to turn off the display of a context reality model, without having to go trough the detach / attach process over and over..

Also, it should be possible to persist this visibility flag part of the view serialization process so that a reality can be loaded again, but remains invisible.

mdastous-bentley commented 1 week ago

@pmconne does it look good to you so far?

pmconne commented 1 week ago

@pmconne does it look good to you so far?

Please explain the motivation in the PR description.

I have concerns that just making draw a no-op will introduce surprising behavior. There is plenty of code that iterates reality models and/or tile trees without drawing them - what are they supposed to do if it's flagged as invisible? (Currently, they will have no idea, and behave as if it is visible). A simple example: fit view will include the extents of any invisible reality models.

mdastous-bentley commented 1 week ago

@pmconne A more brutal approach would to skip the model in DisplayStyleState.forEachRealityModel, but I feel like it could cause confusion too.

pmconne commented 1 week ago

when want to turn off the display of a reality model, without having to go trough the detach / attach process over and over.. Also, it should be possible to persist this visibility flag part of the view serialization process so that a reality can be loaded again, but remains invisible.

Who specifically wants this - a user, or a developer? In what app - iTwin Experience? Are we talking about managing the visibility of scene objects within an "iTwin scene", which is the province of the app, not the core?

MarcBedard8 commented 1 week ago

We want to make a distinction between attached and displayed.

A data source can be attached to a scene (aka display list, aka curated data for this session) An attached data source can be visible or not (display on/off)

iModel support this concept with model selector and we have a reality data model in the iModel which expose that behavior.

But reality data attached to display style doesn’t have that concept. That force user of the API to “detach” that reality data when display need to be turn off. Doing so, we lost every setting (transparency, color, etc..) and have to backup these as user expect same setting when turning “display” back on, which mean that API user need to re-attach the reality data and reset the backup setting.

We believe adding a simple flag to turn on/off display in a view will simplify the API utilisation. Map-Layer already operate that way, and it should not be different for other data source including reality-data.

MarcBedard8 commented 1 week ago

This request come from the "Unification widget" project where we will merge reality-data, map-layer, annotations and feature into the same widgets.

pmconne commented 1 week ago

iModel support this concept with model selector and we have a reality data model in the iModel which expose that behavior.

Invisible context reality models have to follow same behavior as those do. e.g., they don't contribute to the computed scene extents, they aren't included when we iterate tile trees in the view, etc.

pmconne commented 1 week ago

Motivation for adding the visible flag to RealityModelDisplaySettings? Display settings can be applied to context reality models and to persistent reality models. A visible flag doesn't make sense for persistent reality models - their visibility is controlled by the model selector. Did you consider putting the flag on ContextRealityModel instead?

mdastous-bentley commented 1 week ago

forEachRealityTileTreeRef

Doing so will make the reality model behaves like it has not been attached. You think that's a better over a non-op draw?

pmconne commented 1 week ago

forEachRealityTileTreeRef

Doing so will make the reality model behaves like it has not been attached. You think that's a better over a non-op draw?

Isn't that the objective? If you don't skip its tile tree when it's invisible then, for example, fitting the view will cause fit to a volume that includes this invisible reality model. I'd like to see a unit test that proves that ViewState.computeFitRange does not include the bounding boxes of invisible reality models.

What are the circumstances in which you would want to process the tile tree of an invisible context reality model?

mdastous-bentley commented 1 week ago

@pmconne Im testing the suggested skip 'forEachRealityTileTreeRef', and now having a bug when toggling on/off: If the view is changed (zoomed in/out) while the reality model is invisible (and no other content is part of the view), toggling the reality model to invisible=false again wont make it appear UNTIL the view is changed (i.e. zoomed).

For some reasons, when the reality model is back visible, no tiles get selected... RealityTile.computeVisibilityFactor returns -1 because isFrustumCulled is true.

aruniverse commented 1 week ago

If an attached reality model is set to be invisible, and I for example load from a Saved View, does it still make GET reqs to fetch the tiles from the reality data servers?

pmconne commented 1 week ago

If an attached reality model is set to be invisible, and I for example load from a Saved View, does it still make GET reqs to fetch the tiles from the reality data servers?

If we skip processing its tile tree as suggested, no. A unit test can prove this.

mdastous-bentley commented 5 days ago

@pmconne should be good now

mdastous-bentley commented 5 days ago

@Mergifyio backport release/4.9.x

mergify[bot] commented 5 days ago

backport release/4.9.x

✅ Backports have been created

* [#7186 Allow context reality model visibility to be toggled on/off (backport #7170) [release/4.9.x]](https://github.com/iTwin/itwinjs-core/pull/7186) has been created for branch `release/4.9.x` but encountered conflicts