iTwin / itwinjs-core

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

vp.zoomToElements works unexpectedly with MarginOptions #6977

Open Modeeeeeee opened 1 month ago

Modeeeeeee commented 1 month ago

Describe the bug Viewport.zoomToElements has a strange behaviour of not centering the elements that we are zooming to. When passing marginOptions to the function itself (lets say top 0.05, left-right 0.05 and bottom 0.55) you are expecting that the tightest box surrounding those elements would be centered but in reality depending on your rotation and probably some other factors it could be shifted on the bottom of those margins

To Reproduce Steps to reproduce the behavior:

  1. Open an iModel with some elements that you can fit your view
  2. Find an element that you want to fit your view
  3. Call vp.zoomToElements with the ids of those elements and specify margin options (our case - {bottom: 0.55, top: 0.05, left: 0.05, right: 0.05})
  4. Depending on a few factors the elements can be centered, but also there are ways to make the element stick to the bottom or not care about the top margin it has (we have an issue with top safe area not taken into account)

Expected behavior The expected behaviour for zoomToElements having in mind that it calculates the tightest box around those elements is to center that box based on the margins given.

Screenshots https://github.com/user-attachments/assets/33d33944-a2a1-41d1-8d5a-c23ef02f566d https://github.com/user-attachments/assets/753fb4ac-e6af-4988-b41a-5fe70793d26b

Desktop (please complete the applicable information):

pmconne commented 1 month ago

Depending on a few factors the elements can be centered, but also there are ways to make the element stick to the bottom or not care about the top margin it has (we have an issue with top safe area not taken into account)

Need you to be specific about these "ways" and "factors".

Hasn't this been discussed elsewhere? If so please link to that discussion.

Modeeeeeee commented 1 month ago

Depending on a few factors the elements can be centered, but also there are ways to make the element stick to the bottom or not care about the top margin it has (we have an issue with top safe area not taken into account)

Need you to be specific about these "ways" and "factors".

Hasn't this been discussed elsewhere? If so please link to that discussion.

I couldn't find any discussions about zoomToElements and margin options working incorrectly

Talking about those ways or factors, I am guessing that the rotation has the most impact how the elements are zoomed into. If you look into those two videos I attached under screenshots, you can see when the element takes more part of the screen it usually is almost centered, but when it is in a sideways position - it is hard to guess how it will position itself between the margins and usually is zoomed so that it is on the bottom of the margins. Positioning it centered would be ideal for our scenario which we are working with.

pmconne commented 1 month ago

@markschlosseratbentley @danieliborra

danieliborra commented 1 month ago

Added to our to-do list.

eringram commented 1 month ago

I was able to reproduce the issue from the first video in display test app. In this video I am calling zoomToElements with the margins {bottom: 0.55, top: 0.05, left: 0.05, right: 0.05} with the key-in dta zoom selected l=0.05 r=0.05 t=0.05 b=0.55 m=1.

With the first call the zoom positions the element closer to the top of the viewport which is what I would expect if the bottom margin is the largest, at 0.55.

After rotating the view, with the second zoom call the element appears much closer to the middle of the viewport, so the larger bottom margin seems to have less of an effect.

Still investigating into why this happens, just wanted to document another example and how to repro

https://github.com/user-attachments/assets/faf6f58b-e58f-4ea3-b6e7-c2087c8c0aab

eringram commented 1 month ago

@Modeeeeeee So the margin options are specified as a percent (MarginPercent) of the view volume that you want to look at-- so in the case of zoomToElements, a percent of the volume encompassing the elements you are zooming to.

This means that like you observed, for an element taking up more of the screen in some dimension, the margin will be proportionally bigger.

I took some screenshots and made a rough paint diagram to illustrate this. The screenshots on the left are the selected element after zoomToElements was called with no margins, and the right side are after it was called with a bottom margin of 0.5. You can see how the bottom margin is proportional to the screen space height of the element.

zoomtoelements_margin_demo

So I believe the function is working as intended. If you want to achieve a large bottom margin with an element that takes up little vertical space on the screen, the margin would have to be a larger percentage, like 0.8.

A further discussion would be if we want to change how the margins work, or if the documentation on margin options is unclear. Although I feel like this is pretty well explained in the MarginPercent docs. @markschlosseratbentley let me know what you think.

I'm also still not sure what problem is being demonstrated by the 2nd video you posted as the behavior there looks normal to me. Could you elaborate why you included that one?

Modeeeeeee commented 1 month ago

@eringram Here is the link to the item on our side : Bug 1375192: Status update - task elements do not center in devices with notch or punch hole camera

From what you have written I suppose that we then have a wrong idea of how margin percent works. In our use case it is pretty much used to center given elements in the desired screen space. The example I gave you was I think something like this : Untitled-2024-02-21-1038

In the example given we have a red rectangle into which we want to fit the given elements and we had an understanding that margin percent gives you the ability to center the elements by given margins (percentages). So once we call it with those parameters sometimes the elements are centered, but sometimes they are not. And I suppose this behaviour is because its intended use is a little bit different - it moves the elements by the margin of current seen element height and the volume mentioned in MarginPercent is specifically to those elements that are passed to zoomToElements rather than the viewport itself, correct me if I'm wrong?

eringram commented 4 weeks ago

@Modeeeeeee

I suppose this behaviour is because its intended use is a little bit different - it moves the elements by the margin of current seen element height and the volume mentioned in MarginPercent is specifically to those elements that are passed to zoomToElements rather than the viewport itself, correct me if I'm wrong?

Yes, I think this is right. Pretty sure the viewport size isn't taken into account by those margin percentages, or by the padding percent option. In your use case like the diagram you posted, is the large bottom margin for accommodating a UI element like this tasks list? So the viewport size is the entire screen, but part of it is covered by the UI, so you want to center the elements in the uncovered portion?

I could not find any existing functionality in the Viewport class to do this.

@markschlosseratbentley @pmconne do either of you know if there is an existing API that can zoom to a volume and add margins based on the viewport size, rather than a view volume? If not, I can add a new method/adjust an existing API for this or provide a code sample of how to do it.

Modeeeeeee commented 4 weeks ago

@eringram Yupp, exactly as you described, we have UI elements like that tasksList which covers a part of the viewport size so we want the elements to be centered in the part that is uncovered but I am guessing we went for the wrong approach with margin percents as it is not what they actually do

pmconne commented 4 weeks ago

Neither of you has shared your actual code. MarginOptions provides two ways to add extra space, with PaddingPercent being preferred (because it's more predictable) over MarginPercent. Which are you using? @eringram appears to be using MarginPercent - is that what @Modeeeeeee is using?

Are you attempting to use them with the camera turned on in the view? (Per the docs and the implementation, they are ignored if camera is on).

Your current best option is to obtain the element's bounding box yourself and then compute the volume you want to zoom to, accounting for whatever extra space you want at the bottom of the viewport to accomodate your Tasks panel. Then animate the viewport to view that volume.

Modeeeeeee commented 4 weeks ago

@pmconne The video which was attached has the camera turned off, we are using MarginPercent by simply passing it to zoomToElements :

const allOptions = {
  ...options,
  noSaveInUndo,
  animateFrustumChange,
  marginPercent,
};
if (elementsToZoomTo) {
  await vp.zoomToElements(elementsToZoomTo, allOptions);

Regarding the current best option, could you add a code snippet how it would look like to get the bounding box itself and animate the viewport ? Also another question could zoomToElements in the future perhaps have an option to center the elements as a feature request ?

pmconne commented 4 weeks ago

For example code, look at the implementation of zoomToElements.