Skip to content
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

#10322: Fix scale selector to make it compatible with different projections #10344

Merged
merged 9 commits into from
Jul 1, 2024

Conversation

mahmoudadel54
Copy link
Contributor

@mahmoudadel54 mahmoudadel54 commented May 20, 2024

Description

In this PR, the issue of scale selector which was not compatible with different projections is resolved.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Issue

#10322

What is the current behavior?
#10322

What is the new behavior?
Now if user opens the print modal, switches the projection to 4326 for example and select a scale from the scale selector list, it will selected successfully without choosing wrong one.

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

I have noticed some points within this issue:

1- wrong calculate scale zoom level in case user changes projection in the print modal, this causes the noticed wrong calculated scale in DD scales.
If you see this line which calculate the scaleZoom if useFixedScales = true

const scaleZoom = getNearestZoom(newMap.zoom, scales);

it uses getNearestZoom method:
export const getNearestZoom = (zoom, scales, mapScales = defaultScales) => {
const mapScale = mapScales[Math.round(zoom)];
return scales.reduce((previous, current, index) => {
return current < mapScale ? previous : index;
}, 0);
};
and into this method it uses defaultScales is applied at each projection and this defaultScales is the google mercator scales :
const defaultScales = getGoogleMercatorScales(0, 21);

So what I did is passing the scales of the actual correct projection to this method to get the correct calculated scaleZoom to display the correct expected scale in the scales drop down list.

2- Based on the 1st point above, the scale value of the configure print map was calculated wrongly:

const scale = scales[scaleZoom];
. Another point I need to mention is that mapZoom of the map viewer is passed to configurePrintMapProp instead of the calculated zoom value of the print map based on the selected projection here:
layers, newMap.projection, currentLocale);
and here as well
configurePrintMapProp(newMap.center, newMap.zoom, newMap.zoom, scale,

so here I refactored the method to calculate needed values based on the projection value

3- using 'getZoomForExtent' in 'configurePrintMap' method is not the proper way to get the mapZoom of the print map --> in some cases the change bet. main map viewer and the print map viewer >= 2 and this makes change in the print result:

const bbox = reprojectBbox([
newMap.bbox.bounds.minx,
newMap.bbox.bounds.miny,
newMap.bbox.bounds.maxx,
newMap.bbox.bounds.maxy
], newMap.bbox.crs, newMap.projection);
const mapSize = this.getMapSize();
if (useFixedScales) {
const mapZoom = getZoomForExtent(bbox, mapSize, minZoom, maxZoom);
const scales = getPrintScales(capabilities);

so I used reprojectZoom method to calculate print map zoom instead using bbox and mapSize for that:

const zoom = reprojectZoom(newMap.zoom, mapSrs, printSrs);
.

4- when user opens the print panel, the sent projection 'prop' should be set initially to be consistent with the UI [selected projection in DD] and to be not misleading if the shown projection into UI not the initial one.

So I used initiated the projection into useEffect in Projection component at initiating the component:

function changeProjection(crs) {
onChangeParameter("params.projection", crs);
onRefresh();
}
useEffect(() => {
if (enabled) {
changeProjection(projection);
addMapTransformer("projection", mapTransformer);

…different projections

Description:
- fix the bug by using the updated scaleZoom value instead of the projected value
…different projections

Description:
- add helpful comment
@tdipisa
Copy link
Member

tdipisa commented May 20, 2024

@offtherailz I would like if you can schedule a call with @mahmoudadel54 to review issues requirements before moving on with this review.

@offtherailz offtherailz marked this pull request as draft May 21, 2024 14:58
@offtherailz offtherailz removed their request for review May 21, 2024 14:58
@tdipisa tdipisa modified the milestones: 2024.01.01, 2024.01.02 May 29, 2024
…different projections

Description:
- fix not compatablity projections issue
- add unit tests
@mahmoudadel54 mahmoudadel54 requested a review from offtherailz May 30, 2024 08:10
@mahmoudadel54 mahmoudadel54 marked this pull request as ready for review May 30, 2024 08:48
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

  • I noticed that the printing result is still wrong when switching coordinates. It have to be consistent with the map.
screencast-localhost_8081-2024.06.18-18_21_32.webm

Everything have to be consistent. I'll review the code where it is fixed.

  • Other useful information are not clear, please explain them better.

@mahmoudadel54
Copy link
Contributor Author

I noticed that the printing result is still wrong when switching coordinates. It have to be consistent with the map. screencast-localhost_8081-2024.06.18-18_21_32.webm

Everything have to be consistent. I'll review the code where it is fixed.

Hi @offtherailz

After checking this issue, I found out, this is the same currently behavior in print [in DEV or QA] ---> if the print projection is one of the geographic coordinate systems [like 4326] and the 'geodetic' = true into the "overrideOptions" in print cfg, and when I edited 'geodetic' to be false, printing map with geographic projection e.g: 4326 printing works for me.
Honestly I don't know the reason.
This is a demo of my debugging:

geodetic.mp4

@mahmoudadel54
Copy link
Contributor Author

  • Other useful information are not clear, please explain them better.

@offtherailz I have updated the useful information section. I hope it is more clear now

…different projections

Description:
- resolve review comments related to unit tests [replace async/await with then + remove unnecessary async/await]
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

AS we discussed in a call @mahmoudadel54 the PR has still the inonsistencies I notified in the Preview.

As we checked, the preview looks consistent when geodetic is set to false, so our development will do it in that direction, assuming that geodetic is set to false.
Now print plugin allows to send the geodetic parameter via configuration, but it doesn't handle to change the preview to fit the final result.

…different projections

Description:
- fix issues of not consistent map print scales with the current map view
…different projections

Description:
- fix FE issue
@offtherailz
Copy link
Member

offtherailz commented Jun 28, 2024

Now it looks a lot better the preview looks to be correctly in sync with every change.
I'm reporting here in a comment my findings just to keep track of them.
I'll check if these issues of the preview already existed and let you know.

  • Discrepancy between the preview and the scale sent (as we have seen in our meeting).

image

As you can see, even if the scale (200000) is correctly sent to the server, the preview looks to be a little smaller than the effective are printed.
Anyway it doesn't look to be a shift in preview of 1 step. Here the next scale(500000), that previews a bigger area that the previous print

image

So the preview for some reason a smaller than the effective print.

Sometimes nearest with the next scale: Here a sample in 4326

image

Debugging I noticed that the resolutions received by the map in this case are 30 while the scales are 15.
I think it is using the default resolutions. if useFixedScales is set to true, maybe the map should receive the scales converted to resolutions, instead of the default resolutions.

@offtherailz
Copy link
Member

I fixed the preview with this commit.

41d8a2f

Basically now, if useFixedScales is set, the resolutions of the preview map are mapped to the scales provided, and the currentZoom is equal to the scale zoom.

Please @mahmoudadel54 look at my changes to understand what was missing and adjust tests if needed

Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Please fix the tests and do a sert of manual tests more.

…different projections

Description:
- resolve FE failure due to changes
@tdipisa tdipisa merged commit b369af2 into geosolutions-it:master Jul 1, 2024
6 checks passed
@tdipisa
Copy link
Member

tdipisa commented Jul 1, 2024

@ElenaGallo please test this in DEV once deployed and let us know for the backport.

@ElenaGallo
Copy link
Contributor

Test passed, @mahmoudadel54 please backport to 2024.01.xx Thanks

@mahmoudadel54 mahmoudadel54 added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Jul 3, 2024
mahmoudadel54 added a commit to mahmoudadel54/MapStore2 that referenced this pull request Jul 3, 2024
…different projections (geosolutions-it#10344)

* geosolutions-it#10322: Fix scale selector to make it compatible with different projections
Description:
- fix the bug by using the updated scaleZoom value instead of the projected value

* geosolutions-it#10322: Fix scale selector to make it compatible with different projections
Description:
- add helpful comment

* geosolutions-it#10322: Fix scale selector to make it compatible with different projections
Description:
- fix not compatablity projections issue
- add unit tests

* geosolutions-it#10322: Fix scale selector to make it compatible with different projections
Description:
- resolve review comments related to unit tests [replace async/await with then + remove unnecessary async/await]

* geosolutions-it#10322: Fix scale selector to make it compatible with different projections
Description:
- fix issues of not consistent map print scales with the current map view

* geosolutions-it#10322: Fix scale selector to make it compatible with different projections
Description:
- fix FE issue

* Fixed preview

* geosolutions-it#10322: Fix scale selector to make it compatible with different projections
Description:
- resolve FE failure due to changes

---------

Co-authored-by: Lorenzo Natali <[email protected]>
@mahmoudadel54
Copy link
Contributor Author

Test passed, @mahmoudadel54 please backport to 2024.01.xx Thanks

@ElenaGallo Backport is done ---> #10452

tdipisa pushed a commit that referenced this pull request Jul 3, 2024
…ble with different projections (#10344) (#10452)

* #10418: Share tool - the 'Add place mark and zoom to sharing link' option is not applied correctly (#10419)

* #10322: Fix scale selector to make it compatible with different projections (#10344)

* #10322: Fix scale selector to make it compatible with different projections
Description:
- fix the bug by using the updated scaleZoom value instead of the projected value

* #10322: Fix scale selector to make it compatible with different projections
Description:
- add helpful comment

* #10322: Fix scale selector to make it compatible with different projections
Description:
- fix not compatablity projections issue
- add unit tests

* #10322: Fix scale selector to make it compatible with different projections
Description:
- resolve review comments related to unit tests [replace async/await with then + remove unnecessary async/await]

* #10322: Fix scale selector to make it compatible with different projections
Description:
- fix issues of not consistent map print scales with the current map view

* #10322: Fix scale selector to make it compatible with different projections
Description:
- fix FE issue

* Fixed preview

* #10322: Fix scale selector to make it compatible with different projections
Description:
- resolve FE failure due to changes

---------

Co-authored-by: Lorenzo Natali <[email protected]>

---------

Co-authored-by: Lorenzo Natali <[email protected]>
@ElenaGallo ElenaGallo removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Jul 3, 2024
@tdipisa
Copy link
Member

tdipisa commented Jul 3, 2024

@ElenaGallo I've also tested in DEV using test map here and following steps indicated here and all seems to work as expected with this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix scale selector to make it compatible with different projections
4 participants