LineTexBucket: fix out of short range (#220, #529) #532

Gustl22 · 2018-04-26T14:15:33Z

As I mentioned in #220 the lineLength often was out of Short range.
So shapes weren't displayed correctly or reversed.

~~I added a distance value to shader, which now replaces the position calculations of texture. lineLength is further needed to avoid minor artifacts at vertices.~~

In rarely special cases the distance could have a value greater than the Shorts range as it's an euclidean distance out of two Short values. So I added a workaround to split line in those cases.

Works fine with #529, too.

Gustl22 · 2018-04-27T07:04:45Z

I rethought my implementation and found a solution which works without additional distance values.

devemux86 · 2018-04-27T07:22:51Z

vtm/resources/assets/shaders/linetex_layer_tex.glsl

+    // use linelength mod texture step (mod is always positiv)
+    vec4 c = texture2D(tex, vec2(mod(v_st.s, step), (v_st.t + 1.0) * 0.5));
+    float fuzz = fwidth(c.a);
+    gl_FragColor = (c * u_color) * smoothstep(0.5 - fuzz, 0.5 + fuzz, c.a);


Quick comment: please let's keep any formatting changes separately in own PR.
It's important to be able to review now and in the future any core changes easily.

You mean the spaces I added?

Yeah, any improvements in format, refactor, names, comments can perfectly exist separately.
In the future (and now) when we come back to search for a commit's changes, it's very important to see the commit's changes for its specific purpose and only them, not searching in many diffs where exactly is the actual code that changed map rendering.

Agreed. I'm a bit lazy in such matters.

A few comments I added were needed to understand what's going on. Next time I'll try to add the not affected ones separately.

~~I'll add formatting PR when this is merged.~~

# Conflicts: # vtm/resources/assets/shaders/linetex_layer_tex.glsl

devemux86 · 2018-04-30T18:14:41Z

Don't like the result inside maps where the functionality is more important than overlays.
e.g. now the oneway textures "run" like crazy during zoom, while on master they progress very smoothly.

devemux86 · 2018-04-30T18:34:44Z

I merged the PR in branch issue_532 rebased on master to avoid any conflicts.
Can continue working there.

devemux86 · 2018-05-01T08:44:31Z

vtm/resources/assets/shaders/linetex_layer_tex.glsl

    float step = 2.0;
    if (u_mode == 2.0) { // dashed texture
      step = 1.0;
    }
-    vec4 c = texture2D(tex, vec2(abs(mod(v_st.s + 1.0, step)), (v_st.t + 1.0) * 0.5));
+    // use linelength mod texture step (mod is always positiv)
+    vec4 c = texture2D(tex, vec2(mod(v_st.s, step), (v_st.t + 1.0) * 0.5));


Why the remove of the "+ 1.0" in mod?

The better question is, why there was a "+ 1.0" ?
v_st.s is the interpolated lineLength value between two vertices, which is divided with u_pscale.
Theoretically v_st.s can have a range from -∞ to +∞. So it makes no sense to add 1 as it is taken modulo anyway and then has (dependent on dash flag) a range from 0 to 1 or 2.
The only reason could be that texture is shifted so it looks better at the beginning, but I didn't noticed any better look. If leaving + 1.0 we should comment that it has no further meaning.

Seems the answer is found in #536. 🙂

Gustl22 · 2018-05-01T09:49:11Z

Don't like the result inside maps where the functionality is more important than overlays.
e.g. now the oneway textures "run" like crazy during zoom, while on master they progress very smoothly.

Valid objection: The reason is that we now start to count lineLength from Short.MIN_VALUE to use full range.
The good thing is, we can revert the first initialization to 0. When the Short max value is reached, lineLength is reset to min value anyway. But this does not affect one way textures inside base map as they never reach this value cause of clipping. I'll do a PR.

devemux86 · 2018-05-01T10:17:05Z

I tried that before above comment and the result is a bit strange.
Some oneway symbols remain fixed, some move around them.

devemux86 · 2018-05-01T10:25:39Z

BTW there is not anything about "objections" here, just that each and every change needs extensive testing in all library uses so that not break any existing functionality.

Gustl22 · 2018-05-01T11:02:09Z

I'm not a native speaker as you can see 😜
So maybe "objection" was the wrong expression for this.
My intent always is to improve this nice library without regression :D

devemux86 · 2018-05-01T11:39:20Z

The PR concept is certainly nice and seems to solve the "reverse" issue and probably others.
Just wanted to remind (to all of us) that need in each case to make many tests. 🙂

So here since the map textures didn't seem to have issues (?) because of native tiling, we need to not affect them with that change that seems more relevant to overlays.
Like some adjustment is needed where the former code can still work for them?

Gustl22 · 2018-05-01T12:28:49Z

I tried that before above comment and the result is a bit strange.
Some oneway symbols remain fixed, some move around them.

This is the reason of + 1.0, to avoid static line symbols at the beginning, so it does start with a gap and not a symbol (like I mentioned before). Will revert this.

So here since the map textures didn't seem to have issues (?)

It could have, if lineLength in one tile is larger than Short value. E.g. a spiral. But it's very unlikely.

we need to not affect them with that change

It wouldn't be affected, if reverting + 1.0. It does then exactly the same than before (until it hits the critical Short limit). The former code is inherited. To differ these cases is a bit exaggerated in my opinion.

Gustl22 · 2018-05-01T12:36:30Z

See #536

devemux86 · 2018-05-01T13:44:20Z

Very nice, with #536 the regression inside map textures seems fixed!

devemux86 · 2018-05-01T15:24:22Z

Thanks, all seem working now!
Merged via 77cfad5.

Gustl22 · 2018-05-01T16:42:58Z

@devemux86: Thank you for reviewing and bring out the bugs!

devemux86 · 2018-05-01T16:50:19Z

That's the magic of open source projects: users cooperation. 🙂
Thanks for the continuous improvements and fixes!

Gustl22 added 3 commits April 26, 2018 16:01

Fix LineTexBucket: out of short range (mapsforge#220, mapsforge#529)

43de9fd

LineTexBucket: fix datatype of adding to vertexData

1e09d76

LineTexBucket: simplify tex calculations

c87b26d

devemux86 reviewed Apr 27, 2018

View reviewed changes

Gustl22 added 2 commits April 27, 2018 09:43

LineTexBucket: undo formatting

eb467e2

Merge branch 'master' into issue_220

1c2e864

# Conflicts: # vtm/resources/assets/shaders/linetex_layer_tex.glsl

devemux86 added the bug label Apr 30, 2018

devemux86 pushed a commit that referenced this pull request Apr 30, 2018

LineTexBucket: fix out of short range (#532)

921642b

devemux86 reviewed May 1, 2018

View reviewed changes

devemux86 added this to the 0.10.0 milestone May 1, 2018

devemux86 pushed a commit that referenced this pull request May 1, 2018

LineTexBucket: fix out of short range (#532)

77cfad5

devemux86 closed this May 1, 2018

devemux86 mentioned this pull request May 1, 2018

PathLayer (regular) reverses #220

Closed

Gustl22 deleted the issue_220 branch May 1, 2018 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LineTexBucket: fix out of short range (#220, #529) #532

LineTexBucket: fix out of short range (#220, #529) #532

Gustl22 commented Apr 26, 2018 •

edited

Loading

Gustl22 commented Apr 27, 2018

devemux86 Apr 27, 2018

Gustl22 Apr 27, 2018

devemux86 Apr 27, 2018

Gustl22 Apr 27, 2018

Gustl22 Apr 27, 2018 •

edited

Loading

devemux86 commented Apr 30, 2018

devemux86 commented Apr 30, 2018 •

edited

Loading

devemux86 May 1, 2018

Gustl22 May 1, 2018 •

edited

Loading

devemux86 May 1, 2018

Gustl22 commented May 1, 2018 •

edited

Loading

devemux86 commented May 1, 2018

devemux86 commented May 1, 2018

Gustl22 commented May 1, 2018

devemux86 commented May 1, 2018

Gustl22 commented May 1, 2018

Gustl22 commented May 1, 2018

devemux86 commented May 1, 2018

devemux86 commented May 1, 2018

Gustl22 commented May 1, 2018

devemux86 commented May 1, 2018

LineTexBucket: fix out of short range (#220, #529) #532

LineTexBucket: fix out of short range (#220, #529) #532

Conversation

Gustl22 commented Apr 26, 2018 • edited Loading

Gustl22 commented Apr 27, 2018

devemux86 Apr 27, 2018

Choose a reason for hiding this comment

Gustl22 Apr 27, 2018

Choose a reason for hiding this comment

devemux86 Apr 27, 2018

Choose a reason for hiding this comment

Gustl22 Apr 27, 2018

Choose a reason for hiding this comment

Gustl22 Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

devemux86 commented Apr 30, 2018

devemux86 commented Apr 30, 2018 • edited Loading

devemux86 May 1, 2018

Choose a reason for hiding this comment

Gustl22 May 1, 2018 • edited Loading

Choose a reason for hiding this comment

devemux86 May 1, 2018

Choose a reason for hiding this comment

Gustl22 commented May 1, 2018 • edited Loading

devemux86 commented May 1, 2018

devemux86 commented May 1, 2018

Gustl22 commented May 1, 2018

devemux86 commented May 1, 2018

Gustl22 commented May 1, 2018

Gustl22 commented May 1, 2018

devemux86 commented May 1, 2018

devemux86 commented May 1, 2018

Gustl22 commented May 1, 2018

devemux86 commented May 1, 2018

Gustl22 commented Apr 26, 2018 •

edited

Loading

Gustl22 Apr 27, 2018 •

edited

Loading

devemux86 commented Apr 30, 2018 •

edited

Loading

Gustl22 May 1, 2018 •

edited

Loading

Gustl22 commented May 1, 2018 •

edited

Loading