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

Scale factor for short vertices calculation #537

Closed
devemux86 opened this issue May 1, 2018 · 28 comments
Closed

Scale factor for short vertices calculation #537

devemux86 opened this issue May 1, 2018 · 28 comments
Milestone

Comments

@devemux86
Copy link
Collaborator

Can provide some automatic MapRenderer.COORD_SCALE in high resolution screens, e.g. in MapView initialization, like do currently with Tile.SIZE.
Preferably that should be optional via Parameters, so that users can use their own in the apps.

Related to #343.

@devemux86
Copy link
Collaborator Author

Could use device dpi as guide and set MapRenderer.COORD_SCALE after any CanvasAdapter.dpi is set.
Probably keep current "8" scale as default and use "4" for high resolution / dpi screens.

@devemux86 devemux86 changed the title MapRenderer COORD_SCALE parameter Scale factor for short vertices calculation May 5, 2018
@devemux86
Copy link
Collaborator Author

@Gustl22 @Longri
Can you test, any opinions for the current 420dpi limit?

@Longri
Copy link

Longri commented May 5, 2018

On my App I use COORD_SCALE = 1
And I have never seen any problems!
So I think it gives no problems with calculated COORD_SCALE, if the result >1

@devemux86
Copy link
Collaborator Author

Default COORD_SCALE = 8 seems working on Desktop and Browser.
Full screen tests on 4K or 5K screens would be interesting to check. 🙂

On Android the scale 8 seems ok until 420dpi wherever was tested so far.
Above that dpi the high resolution devices require smaller scale and 4 is a sane default as next step.

@Longri
Copy link

Longri commented May 5, 2018

I have only Full HD Devices!

@devemux86
Copy link
Collaborator Author

In #343 you mentioned a Note 4 1440x2560 px (~518ppi), is it available?
Or else Android emulator at 560dpi clearly shows the improvement with scale 4.

@Longri
Copy link

Longri commented May 5, 2018

You are right! I will check again. But with COORD_SCALE = 1 is all fine. On Note 4!

@Gustl22
Copy link

Gustl22 commented May 5, 2018

I'll test with 4k on Monday.

@Longri
Copy link

Longri commented May 7, 2018

It looks good on Galaxy Note4

Also on iOS (as dependency on my own App! iOS-example can't run on AndroidStudio)

@Longri
Copy link

Longri commented May 7, 2018

But I have a scaling problem with my app!
After upgrade my app to the last vtm-snapshot, something happened with the scaling of the map elements.

The scale bar is the right size, but all map elements are too small. (On Galaxy Note4)

Which properties must/can I set?
1

At the moment I set

            float scaleFactor = CB.getScaledFloat(Settings.MapViewDPIFaktor.getValue());
            CanvasAdapter.dpi = CanvasAdapter.DEFAULT_DPI * scaleFactor;

@Longri
Copy link

Longri commented May 7, 2018

Debug Breakpoit : CanvasAdapter.dpi = 499.99872

@devemux86
Copy link
Collaborator Author

devemux86 commented May 7, 2018

Don't remember changing something related to map scaling lately. 🙂

But no need to set CanvasAdapter.dpi, it's set automatically on initialization.
As well as the Tile.SIZE (see here), unless want to set it explicitly.

EDIT: or see here for Android with libGDX.

@Gustl22
Copy link

Gustl22 commented May 7, 2018

We have to find a workaround to declare COORD_SCALE as final! There're some (final) constants which are initialized with first COORD_SCALE = 8.0f and aren't updated.
See AbstractVectorLayer.MAX_CLIP, LineBucket -> Renderer.COORD_SCALE_BY_DIR_SCALE, S3DBLayer.TILE_SCALE, (regular) PathLayer.MAX_CLIP.

Or find a way to handle them dynamically.

I only mention S3DBLayer. Not sure, if I made a fault there.

But MAX_CLIP should be updated to profit from the larger clip range.

I tested on 4k, but I only have a PPI of 157. So the clip is not enlarged and wouldn't be updated cause constants remain the same. This especially affects paths (and textures) with tilt up to 80 degree. So paths are cut there with regular PathLayer.

bildschirmfoto von 2018-05-07 20-40-47

Only as question: is there a reason why low dpi devices (or very large devices with medium dpi, like 4k) cannot use 4 as COORD_SCALE ?

@devemux86
Copy link
Collaborator Author

MapRenderer.COORD_SCALE is a public static variable, could also set it externally before any library initialization, like doing with various Parameters.
Just that way the work is transferred to application from the library.
Wouldn't want to complicate all library structure just for that, need to think more the implementation.

is there a reason why low dpi devices cannot use 4 as COORD_SCALE?

No particular, just usually prefer to not change defaults that are already well / long tested.

@devemux86 devemux86 reopened this May 7, 2018
@Longri
Copy link

Longri commented May 8, 2018

First:
The scaling problem was my mistake. I have preload a Theme bevore CanvasAdapter.dpi was calculated. SORRY!

Second:
I use COORDSCALE=1, every time, every Device!
So don't set FINAL (or set Final to 1)

@Gustl22
Copy link

Gustl22 commented May 8, 2018

I meant final in terms of: once set (even if it's set by user) and never should be changed (in app) afterwards. But that's not possible for static vars, I think. So need to think more dynamically for the dependent constants :D

@devemux86
Copy link
Collaborator Author

If developer of an app "knows" and changes an "advanced" parameter, why will change it again and possibly break it? It's not logical and we're not here to cover such "strange" workflows.

Just improving things without complicate the library more.
When have more time, all these need more testing..

@devemux86
Copy link
Collaborator Author

@Gustl22 after debugging the variable values while the samples are running, I cannot find any issues with current implementation.

MapRenderer.COORD_SCALE is initialized properly before any of all those classes, so they use the new calculated scale (if any).
It's the exact same process like Tile.SIZE, which works nicely all this time.

Also those variables can be overridden via Parameters and user sets its own before library initialization.

@devemux86
Copy link
Collaborator Author

Calculation was added on Android: regular + libGDX implementations.
iOS could use probably common dpi check and scale factor.

Desktop or Browser may need another check, due to different dpi / resolution.

@Gustl22
Copy link

Gustl22 commented May 8, 2018

Sorry my bad! I thought it was added to all environments. I changed the values in calculateCoordScale() and wondered why they weren't updated in the constants.... on Desktop.

Can we add this to Desktop, too, with lower dpi values?

@devemux86
Copy link
Collaborator Author

devemux86 commented May 8, 2018

Sure, I don't have a 4K screen (or similar) available to test now, do we need to use also dpi there and which threshold?

@Gustl22
Copy link

Gustl22 commented May 8, 2018

Ok I made various tests, always with Viewport.MAX_TILT = 80 and regular PathLayer.

  • Desktop 1920 × 1080, unknown ppi/dpi , COORD_SCALE = 8: works fine (minimal clipped)

  • Desktop 3840 × 2160, 157 ppi, 96 dpi (software), COORD_SCALE = 8: clipped (see above)

  • Android (Smartphone) 1920 × 1080, 432 ppi, 480 dpi, COORD_SCALE = 4: works fine

  • Android (Tablet 4:3) 2048 × 1536, 264 ppi, 320 dpi, COORD_SCALE = 8: clipped
    screenshot_20180508-160720

Desktop does not provide (the right) software density information, cause screen density couldn't be read by system (so always set to 96). So we have to fall back to screen resolution.

My Android Tablet clips the paths: although it has a very high resolution, the density isn't reached.

So I would propose to make all dependent of screen resolution (or a mixture of both) as this defines the range for later OpenGL calculations. As limit we can use dimensions equals and above 1920 × 1080 pixels for COORD_SCALE = 4.0f.

@devemux86
Copy link
Collaborator Author

Note that tilt 80 is not default and we're talking here about default values.
Or else each user can change tilt limits and scale value as wish according to specific needs.

We just need sane defaults testing with inside default settings.

@devemux86
Copy link
Collaborator Author

BTW I tested on an Nexus 9 with above specs and didn't see something strange with default settings.

@devemux86
Copy link
Collaborator Author

devemux86 commented May 8, 2018

So you think the threshold could be if smaller dimension is over 1080px?

@Gustl22
Copy link

Gustl22 commented May 8, 2018

That sounds good.

For current 65 degree I can't see any issues (with all devices).

@devemux86 devemux86 reopened this May 10, 2018
@devemux86
Copy link
Collaborator Author

I pushed new implementation for Android and globally for all libGDX based platforms.

@Gustl22
Copy link

Gustl22 commented May 11, 2018

Works nice! I noticed some clipping on my Phone (1080 width) with higher tilt, but I think we cannot cover all cases and certainly it's customizable, so it's fine! Thanks :)

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

No branches or pull requests

3 participants