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

Add ZoomLimiter to LabelLayer (#504) #544

Closed
wants to merge 1 commit into from

Conversation

Gustl22
Copy link

@Gustl22 Gustl22 commented May 25, 2018

I don't really know if zoom limiter is necessary in LabelLayer. Maybe it's good to have. But since using TileSeparator the labels aren't displayed twice at tile borders.
And don't know if these few labels, which aren't processed multiple times (at borders) have better performance economization compared to loading the zoom limit tiles of LabelLayer (here zoom 18).

Anyway I made LabelLayer.ZOOM_LIMIT = 18 public, so if set this to Viewport.MAX_ZOOM_LEVEL all works similar to before.

@devemux86
Copy link
Collaborator

devemux86 commented May 25, 2018

It's not only about labels / symbols at tile edges.
It's also how they are multiplied for each zoom e.g. >17.
If set symbols at polygon parkings, then at large zooms there are multiple symbols per "tiled" polygon.


static final Logger log = LoggerFactory.getLogger(LabelLayer.class);

static final String LABEL_DATA = LabelLayer.class.getName();

private static final long MAX_RELABEL_DELAY = 100;

/* Set to maximum "zoom-min" value for labels in render themes */
public static final int ZOOM_LIMIT = 18;
Copy link
Author

Choose a reason for hiding this comment

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

Of course have to make it private or remove final ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

ZOOM_LIMIT shouldn't be large or else rendering is similar like now. A sane default could be 17.

Nevertheless a side effect is that if set in themes zoom-min > ZOOM_LIMIT then the labels / symbols are not rendered!
We somehow need to not break themes functionality here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added in my tests an extra constructor with also the zoom limit.

@Gustl22
Copy link
Author

Gustl22 commented May 26, 2018

If set symbols at polygon parkings, then at large zooms there are multiple symbols per "tiled" polygon.

Yeah that's a good reason; for other areas, too.

Unfortunately the zoom limit has to be 18, otherwise it is bothered by render themes.

@devemux86 devemux86 modified the milestone: 0.10.0 May 30, 2018
@devemux86
Copy link
Collaborator

I merged it for now in issue_544 branch with mentioned improvements.

Will need more work there to not break themes zoom-min experience.

@Gustl22
Copy link
Author

Gustl22 commented Jun 1, 2018

Will need more work there to not break themes zoom-min experience.

I didn't work with theme logic so far. I'll have a look at it next week when I have enough time (or can try to solve yourself, if you want)

@Gustl22
Copy link
Author

Gustl22 commented Jun 12, 2018

I'm afraid. I don't see a proper way to implement that. In Rule there's a method matchElement.
But it can't differ for which RenderStyle or here the LabelLayer it should match. Somehow we have to set equal LabelLayer and Symbol + Text + Caption render styles.
So we would need to request Layer information, which isn't available here. We also can't differ, if tile has original zoom level or faked overzoomed level. So it would match for non-overzoomed layers, too.
Even if we manage that, we have to change VectorTileLoader's renderText and renderSymbol to call theme hooks on zoom limit tiles if it's overzoomed. But here we don't get these infos, too.
Even then, I didn't manage to display captions, in our example (I hardcoded the other stuff to test).
I run out of ideas 😟

Main problem is that tiles are independent of Layers, and call information only of its zoom level. So we only could use zoom limits, when nothing in zooms or themes changes in their layers, so it uses exactly what we expect. Tiles can't differ the layers and individually pass information for their needs.

I would suggest, to use zoom limit at 18, where nothing in theme's changes. So symbols and captions aren't divided in 19 and 20.
And can personally increase if want to use higher zooms in themes.
Instead we should fix label positioning, while map creation(?). This would work precisely in any case.

Sorry for that, if you have more ideas, let me know.

@devemux86
Copy link
Collaborator

I would suggest, to use zoom limit at 18, where nothing in theme's changes. So symbols and captions aren't divided in 19 and 20.

Default implementation should probably be without zoom limit on themes, that's what users expect.

Instead we should fix label positioning, while map creation(?). This would work precisely in any case.

I already did that in mapsforge/mapsforge#1061. 🙂
Now can use label-position safely, either globally as map-writer option or per map element in tag-mapping file.

@devemux86
Copy link
Collaborator

@Gustl22 thanks for double checking it!

@Gustl22
Copy link
Author

Gustl22 commented Jun 12, 2018

Default implementation should probably be without zoom limit on themes, that's what users expect.

So we could set LabelLayer.ZOOM_LIMIT = Viewport.MAX_ZOOM_LEVEL, so it would work as there wasn't a ZoomLimiter.

I already did that in mapsforge/mapsforge#1061.

Very nice. Does it make sense for other polygons/areas, too? Or we individually can check while testing, and change from time to time.

@devemux86
Copy link
Collaborator

devemux86 commented Jun 12, 2018

So we could set LabelLayer.ZOOM_LIMIT = Viewport.MAX_ZOOM_LEVEL, so it would work as there wasn't a ZoomLimiter.

Something like that. In issue_544 branch this default value is private and can control the label layer via a new constructor.

Does it make sense for other polygons/areas, too?

What do you mean?
The label-position calculation can be applied on all map areas - if set as map-writer option.
Or per map element - if set in tag-mapping.

@Gustl22
Copy link
Author

Gustl22 commented Jun 12, 2018

What do you mean?

I only meant if it's necessary to change label-position defaults, except at buildings, e.g. amenity parking? Especially on large areas, we can't set multiple label positions. But I don't think essential changes are needed.

@devemux86
Copy link
Collaborator

if it's necessary to change label-position defaults, except at buildings, e.g. amenity parking

That would be useful, feel free to see what other closed ways - which host labels or symbols - can use the label-position in tag-mapping.

I only added it in buildings as the most relevant example for testing.
e.g. lakes usually don't need it in order to allow multiple labels(?)

@devemux86 devemux86 added this to the 0.10.0 milestone Jun 24, 2018
@devemux86
Copy link
Collaborator

Thanks!
Merged via 9e52e9e, keeping zoom limit to max zoom to allow render themes to work.

@devemux86 devemux86 closed this Jun 24, 2018
@Gustl22 Gustl22 deleted the labels_zoomlimiter branch June 24, 2018 14:54
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.

2 participants