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

Curved pathText rendering, increased number of road names (#885) #1112

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
2 participants
@b3nn0
Copy link

b3nn0 commented Mar 28, 2019

Hi,
first try of an implementation to fix #885.
Current remaining issues:

  • TextStroke is taken from here: http://www.jhlabs.com/java/java2d/strokes/
    Interestingly, the websites title is "Fun with Java2D". Weird. I can't feel the fun at all ;)
    It does currently not obey the MapsForge coding style. Also I have changed two or three lines in the file to make it work correctly for this purpose. How should I go about this?...
  • Sometimes, there is some label collision happening. E.g. for multi-lane highways. No idea why:
    grafik
  • Unit tests. I can't seem to get them working. For Android, I use Android Studio. For the AWT version no idea what to use. I'm debugging with VSCode, but it somehow can't find JUnit, so I can't run the unit tests. How to do a proper dev environment setup for AWT?

Other than that, it looks quite decent. Finally, one can also read some street names in the lower zoom levels. Personally, I'd even go one lower with the min-levels in the default render theme (but let them repeat less often).

Take this as an RFC, not a final patch.

Adrian Batzill
@devemux86

This comment has been minimized.

Copy link
Collaborator

devemux86 commented Mar 29, 2019

Thanks, will review it.

Unit tests

Mapsforge development can be done entirely inside Android Studio for all platforms.
Also tests exist already in several modules, like mapsforge-map, mapsforge-map-awt, etc.

Adrian Batzill
@b3nn0

This comment has been minimized.

Copy link
Author

b3nn0 commented Mar 29, 2019

Ahh, thanks for the hint with Android Studio. Now it's working fine and I can run the tests.
Let me know what you think of the general approach / code layout. When this is somewhat final, I will start writing a couple of unit tests. especially for the LineString class that seems to be required.

EDIT: Also notable: For extreme testing, I set the displayModel's userScaleFactor to 5, so I get huge labels and have an easier time finding problem cases.
What I noticed: in this mode, I get quite a few cut-off labels at tile-boundaries. Should I add code to not draw labels at tile boundaries? Or is there a better way to deal with this?

@devemux86

This comment has been minimized.

Copy link
Collaborator

devemux86 commented Mar 29, 2019

About pull requests, it's recommended to use a topic branch, instead of working locally on master branch. See also GitHub documentation for more details.

Regarding cut labels, Mapsforge already handles gracefully labels & symbols at tile boundaries, either with regular rendering or with label layer.
So if in pull request's code now it's not working anymore, then something broke library's correct behavior and that must not happen.
Also such large scale factor is of no practical use without proper increase of tile size too.

@b3nn0

This comment has been minimized.

Copy link
Author

b3nn0 commented Mar 29, 2019

About separate branch: Yeah I know. I've fallen into that trap before ;) However, I'm not planning any other mapsforge development right now, so I think it's fine.

I've just pushed another set of changes.

  • Unit test for the new LineString class
  • Fixed the broken and overlapping labels. It was due to a typo in the bounding box computation.

I would now consider the technical side pretty much done and ready to be merged. If you don't mind, I will do some more changes to the default and osmarender themes to reduce the number of labels a bit.
Now that we don't rely on long line segments any more, there are quite a lot of labels now, so increasing the repeat-gap a bit seems reasonable.

One more screenshot, just for the nice looks:
image

@devemux86 devemux86 added this to the 0.12.0 milestone Mar 30, 2019

import java.util.List;

public class LineString {
private List<LineSegment> segments = new ArrayList<>();

This comment has been minimized.

Copy link
@devemux86

devemux86 Mar 30, 2019

Collaborator

Can expose segments and avoid implementing many access methods.


Path2D getRawPath() {
return this.path2D;
}

This comment has been minimized.

Copy link
@devemux86

devemux86 Mar 30, 2019

Collaborator

path2D is already accessible where is necessary.

t.setToTranslation(x, y);
t.rotate(angle);
t.translate(-px - advance, -py);
t.translate(0, glyphVector.getVisualBounds().getHeight() / 2); // move slightly down to

This comment has been minimized.

Copy link
@devemux86

devemux86 Mar 30, 2019

Collaborator

Labels are still not centered, because of way text container was made larger by text height.

devemux86 added a commit that referenced this pull request Mar 30, 2019

devemux86 added a commit that referenced this pull request Mar 30, 2019

@devemux86

This comment has been minimized.

Copy link
Collaborator

devemux86 commented Mar 30, 2019

Thanks, very nice work!

Squashed and merged via d5359eb, with format and sort methods improvements.

I pushed via 516dd6c the fix for proper center the labels also on desktop.

@devemux86

This comment has been minimized.

Copy link
Collaborator

devemux86 commented Mar 30, 2019

Now that we don't rely on long line segments any more, there are quite a lot of labels now, so increasing the repeat-gap a bit seems reasonable.

That could be handled:

devemux86 added a commit that referenced this pull request Mar 31, 2019

@devemux86

This comment has been minimized.

Copy link
Collaborator

devemux86 commented Mar 31, 2019

I changed repeat gap to 100 and we'll see how it appears.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.