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

Mapml vector tiles - early test #363

Closed
wants to merge 6 commits into from

Conversation

aaime
Copy link
Member

@aaime aaime commented Mar 7, 2024

This is a PR allowing for early test of vector tiling with WMS (no caching yet, and fixed styling).
In order to test a layer set it up to be both tiled and using features:

image

With the current code, the boundaries are normally drawn properly but the fill is broken.
I've initially tried to follow the rules we discussed by mail, but they systematically led to bits missing on the sides and the fill was broken anyways and so... I've decided to stop bleeding hours trying to figure out the "rules of the game" and ask for some verification instead. I'd be also happy to receive written-down, firm rules on how a polygon boundary with hidden sides should be represented, and then adhere to that in code and tests (as indicated, the ones discussed by mail do not seem to be working properly).

Here are some examples of misbehavior from GeoServer demo layers:

tasmania_state_boundaries

image

giant_polygon (zoom in once)

image

restricted (zoom in once)

image

countries (zoom in once):

image

This last one is the only case where I see sides that don't belong pop up. I've tried to restrict the layer using a CQL_FILTER, &CQL_FILTER=ADMIN='Croatia', and checked the tile contents, I don't see ordinates going up to the north pole...

The code has comments as to what I've tried so far and the bits that I've removed are just commented out, for ease of testing.

@prushforth could you have a look?
Btw, if you do, I'd recommend disabling HTTP caching and work with the developer tools open, with cache disabled there too, for good measure.

Checklist

For core and extension modules:

  • New unit tests have been added covering the changes.
  • Documentation has been updated (if change is visible to end users).
  • The REST API docs have been updated (when changing configuration objects or the REST controllers).
  • There is an issue in the GeoServer Jira (except for changes that do not affect administrators or end users in any way).
  • Commit message(s) must be in the form [GEOS-XYZWV] Title of the Jira ticket.
  • Bug fixes and small new features are presented as a single commit.
  • Each commit has a single objective (if there are multiple commits, each has a separate JIRA ticket describing its goal).

turingtestfail and others added 3 commits March 6, 2024 13:51
wildcard import removed

abstract parent needs the mime

file license

cleanup

fixed test issues with projection

validating sld filters

added getStyleQuery test

Moved to util class

cleanup

test fix

removed unnecessary inheritance

cleanup

pr response

PR responses and fix to resource vs layer metadata issue

html version now adds feature kv

html source link should return mapml not features

added cql-filter test to sld filter test
Comment on lines +143 to +166
// Currently adding all points with duplications across the visible and non
// visible lines. The boundary is normally fully drawn this way, but the filling
// is not working correctly all the time, and the client sometimes draws lines that
// do not exist. The old code that tried to avoid duplications has been commented
// out, was failing in case there is a visible segment with only two coordinates,
// that were also used by two nearby invisible segments. Maybe we should make
// up a non existing coordinate in the middle in that case?

// Visibility changed, close current span and start a new one.
// The invisible span must be fully contained in the map-span, without duplicate
// ordinate, so remove the last point from the previous span if it's invisible
// However, if a visible segment is enclosed between two invisible ones, we
// cannot leave its coordinates completely out, or it won't display at all...
// if (!nextVisible && currentSpan.size() > 1) {
// currentSpan.remove(currentSpan.size() - 1);
// }
result.add(new TaggedCoordinateSequence(visible, currentSpan));

// if (nextVisible) {
// currentSpan = new ArrayList<>(Arrays.asList(segment[1]));
// } else {
currentSpan = new ArrayList<>(Arrays.asList(segment[0], segment[1]));
// }
visible = nextVisible;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the bit that tried to figure out which coordinates to put in which span. In the end I had to comment them out to get the boundary to show in a (mostly) reliable way.

@prushforth
Copy link
Collaborator

prushforth commented Mar 7, 2024

First quick comment: if you remove the <map-span> elements and take the text value of the coordinate string, it should parse as coordinates. That means that

<map-span class="bbox">-22.5 78.75 -11.25 78.75 -11.25 90</map-span> -11.25 90 -22.5 90<map-span class="bbox">-22.5 90 -22.5 78.75</map-span>

will parse to: -22.5 78.75 -11.25 78.75 -11.25 90 -11.25 90 -22.5 90-22.5 90 -22.5 78.75

and the result is math errors.

@aaime
Copy link
Member Author

aaime commented Mar 7, 2024

@prushforth I find that surprising, is there some general HTML rule that this behavior is based on?
In GML spaces are irrelevant for example (but that's proper XML).

@prushforth
Copy link
Collaborator

I believe so textContent is what we are relying on. https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent

Will get back to it in a minute but I think there's a bug in the viewer rendering besides that issue.

@prushforth
Copy link
Collaborator

In GML spaces are irrelevant for example (but that's proper XML)

IIRC this is also the case in XML when you use the text value of a node it seems to just remove the tags, doesn't do whitespace fixup or something like that. GML is an XML Schema driven thing and doesn't like mixed content.

@prushforth
Copy link
Collaborator

Locally fixed the rendering bug, with help from @AliyanH:
image

Will work on getting our release out.

@prushforth
Copy link
Collaborator

I noticed when playing with the states layer that some polygons were serialized with no geometry, which was causing problems with the tile renderer. I will try to fix that issue, but it might be good to eliminate map-features with no child map-geometry elements at source.

@aaime
Copy link
Member Author

aaime commented Mar 8, 2024

@prushforth yes, we definitely don't want to have emtpy geometries... I've already made some effort to clean out the empty ones, but I guess some are still going out of the gates. I'll see if I can spot them, and fix.

@prushforth
Copy link
Collaborator

To be clear, I tried the tiling features in the OSMTILE projection, and there were a few <map-feature><map-properties>...</map-properties> </map-feature> i.e. no geometry at all, not just an empty geometry (which is a different test case now that I think of it).

@aaime
Copy link
Member Author

aaime commented Mar 8, 2024

I see. In those cases the feature should be skipped entirely. Going to check what's happening.

@prushforth
Copy link
Collaborator

. In those cases the feature should be skipped entirely.

Exactly

@aaime
Copy link
Member Author

aaime commented Mar 8, 2024

I've added a change to skip the features whose geometry is empty or missing entirely.
Are we good in terms of what the server is generating? If so, I'll "tighten the bolts" by adding more tests.

Doing some tests I've noticed a couple of things that seem client-side issues. When rendering point layers, the pont symbol can be clipped to the tile. This is sf:archsites, point symbols are clipped:

image

I believe I'm seeing the same with line layers, with artifacts that might be popping up at tile borders, see here sf:roads comparing vector and png rendering:

image

image

@prushforth
Copy link
Collaborator

I've added a change to skip the features whose geometry is empty or missing entirely.
Are we good in terms of what the server is generating? If so, I'll "tighten the bolts" by adding more tests.

Yes, I belive so. Those issues do look like client rendering problems. I will look into it a bit, but I'm putting my efforts into getting the current viewer released and then to create a PR for GeoServer to update the viewer before the 2.25.0 release, if that is possible.

@aaime
Copy link
Member Author

aaime commented Mar 8, 2024

So the release is on the 18th, 10 days from now. At the same time, we need to get the current open PRs in this repo, plus one (the styling one) in to meet the Milestone 2 deadline. It's starting to feel a bit tight, isn't it?

@prushforth
Copy link
Collaborator

It's starting to feel a bit tight, isn't it?

I can imagine but I think you guys are doing great. No worries from my POV.

@prushforth
Copy link
Collaborator

prushforth commented Mar 9, 2024

I noticed in the tasmania_cities layer when "Use features" is checked, I get a <point> feature inside a map-geometrycollection. It should be map-point:
image

Could be a long-standing bug, maybe I forgot to change the code when we changed to custom elements' map- prefix.

Edit: even if that's fixed, I notice that the client code forgot to code for map-geometrycollection, so it might still not work. I will update the client asap.

@aaime
Copy link
Member Author

aaime commented Mar 11, 2024

Ok, with the last fix the tasmania layer should not be generating a geometry collection anymore.

@prushforth
Copy link
Collaborator

Ok, with the last fix the tasmania layer should not be generating a geometry collection anymore.

I feel like I cheated because I didn't actually fix the viewer yet and it's working. Thanks!

I think that when we send a <map-link rel="tile" type="text/mapml">, we should a) also send a <map-link rel="query" tref="...">, per the usual method. We (Maps4HTML community group) don't yet have a good plan for how to deal with feature attributes in the client in a tiled-mapml situation, so they are just baggage and b) map-properties could be omitted from the response, opting for the query link as a lighter-weight option.

@aaime
Copy link
Member Author

aaime commented Mar 14, 2024

Closing, will create a new PR that's up to date with the current GeoServer code base (after the merge of filtering for vector mapml tiles)

@prushforth
Copy link
Collaborator

OK thanks. Don't know if you saw this issue, but it exists on main as well. Thanks!

@aaime aaime closed this Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants