-
Notifications
You must be signed in to change notification settings - Fork 398
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
#9018: Dashboard - Zoom in/out on maps connected to a 3D map #10460
base: master
Are you sure you want to change the base?
Conversation
…3D map Description: - fix inconsistently issue of zoom in/out that happens in 3d map sync with another map [2d or 3d]
…3D map Description: - remove unnecessary line space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mahmoudadel54 @tdipisa I did a local test with this implementation and there are still problems with the zoom, see video below:
dashboard-zoom.mp4
I've also investigated on the reason of this behaviour and I noticed that the camera event moveEnd it's triggered twice everytime a view has been changed on 3D map, unfortunately I was not able to find the reason of this and I think additional time is needed to understand the problem and find a proper solution.
For the moment we could keep the hardening done on center and zoom.
newProps.viewerOptions?.cameraPosition?.longitude ?? newProps.center.x, | ||
newProps.viewerOptions?.cameraPosition?.latitude ?? newProps.center.y, | ||
newProps.viewerOptions?.cameraPosition?.height ?? this.getHeightFromZoom(newProps.zoom ?? 0) | ||
newProps.mapStateSource !== this.props.id ? newProps?.center?.x ?? newProps.viewerOptions?.cameraPosition?.longitude : newProps.viewerOptions?.cameraPosition?.longitude ?? newProps.center.x, | ||
newProps.mapStateSource !== this.props.id ? newProps?.center?.y ?? newProps.viewerOptions?.cameraPosition?.latitude : newProps.viewerOptions?.cameraPosition?.latitude ?? newProps.center.y, | ||
newProps.mapStateSource !== this.props.id ? this.getHeightFromZoom(newProps.zoom ?? 0) ?? newProps.viewerOptions?.cameraPosition?.height : newProps.viewerOptions?.cameraPosition?.height ?? this.getHeightFromZoom(newProps.zoom ?? 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please restore this to the previous arguments, if we want to propertly control the camera direction we will need to include the viewerOptions property to the widgets dependecies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot to add request changes
see #10460 (review)
@mahmoudadel54 let's keep this on hold for a moment. Wait for my feedback, please. Thank you @allyoucanmap. |
Description
This PR includes fixing the inconsistently issue of zoom in/out that happens in 3d map sync with another map [2d or 3d].
The issue in 3D maps was that
dependenciesToMapProp.js
file updatescenter
andzoom
for map object into widgets state for the synced maps, and due that update_updateMapPositionFromNewProps
method into Map.js of cesium is called to update the map view.The issue was in these lines:
MapStore2/web/client/components/map/cesium/Map.jsx
Lines 504 to 511 in 2d097ff
It does not take the new props of center that was updated within
dependenciesToMapProp
as mentioned above.I have added 2 enhancements:
MapStore2/web/client/components/map/cesium/Map.jsx
Line 489 in 2d097ff
as during test I tried to move from a point with negative coordinates to a point with positive coordinates and vice versa so if the result with negative value, the logic of
isNearlyEqual
will not be accurate as expected.Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
Issue
#9018
What is the current behavior?
#9018
What is the new behavior?
Now the zoom restriction that seems in the issue is fixed
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information