Port asymmetric viewport rendering to gl-js by arindam1993 · Pull Request #8638 · mapbox/mapbox-gl-js

arindam1993 · 2019-08-15T01:35:33Z

Addresses the following issues:
#8328
#4268
and ports @astojilj 's amazing work to the js side.

API:

Users can now specify global left, right, top and bottom padding values (in pixels) in in a padding field in camera functions i.e jumpTo, easeTo and flyTo.
Example:

map.easeTo({{
    padding: {
         left: 400,
         right: 100
    } 
)};

Similar to other camera options the existing padding value is retained if a parameter is unspecified in the call.
Also, similar to @astojilj 's design, the rendering changes are implemented accounting for the offset center elements 8 and 9 of Transform#projMatrix, in Transform#_calcMatrices(), while ensuring that everything that reads from Transform#getCenter doesn't break with the center being offset.

Testing:

Unit tests for EdgeInsets class that tests its behavior in isolation
Unit tests in camera for testing if its interface oh setting padding behave as expected.
Added debug rendering for padding:

Left padding rendered in blue,
Bottom padding rendered in green
Right padding rendered in magenta
Top padding rendered in red
This can be turned on with the map option showPadding.
Add render tests with showPadding set to true

Demo:

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
manually test the debug page
address overlap between padding in fitBounds and padding for camera options for asymmetric viewport
Map API functions such as easeTo and flyTo now support padding: PaddingOptions which lets developers shift the center of perspective for a map when building floating sidebars.

astojilj · 2019-08-19T18:30:09Z

@arindam1993
There is a follow up work that needs to be ported:
mapbox/mapbox-gl-native#15195

It prevents out of memory (too many tiles loaded) with large top padding and prevents displaying horizon - aswe need to define design and interaction with horizon.

astojilj

partial review. continuing tomorrow. Looks nice so far.

astojilj · 2019-08-19T18:58:02Z

        this._calcMatrices();
    }

+    get padding(): EdgeInsetJSON { return this._edgeInsets.toJSON(); }


Serving the existing API in native, i didn't like that in Android we use "padding" and in iOS "edgeInsets". is padding here added to keep it close to native implementation?

I just did it so the public facing term for is it simpler, but happy to change it to whatever.
Maybe @chloekraw , do you have any thoughts?

For this feature, I think padding is the term that's going to be closest to what web developers who use CSS would expect. inset is most commonly used as a type of border style:

https://developer.mozilla.org/en-US/docs/Web/CSS/padding

https://developer.mozilla.org/en-US/docs/Web/CSS/border-style

I did find this https://developer.mozilla.org/en-US/docs/Web/CSS/inset "experimental technology" but it sounds like this inset is more about defining margins around blocks of text and is less analogous to a viewport. (related: https://developer.mozilla.org/en-US/docs/Web/CSS/inset-block, https://developer.mozilla.org/en-US/docs/Web/CSS/inset-inline)

@arindam1993 are there other parts of the GL-JS code base that also use this idea of padding?

@arindam1993 , related to this - what's your opinion about propagating CSS style padding specified to Map's parent HTML container to transform?

@chloekraw I don't think so.

@astojilj I don't think we should, I think devs waning to create floating UI elements, which this feature is designed for would want to set a padding on the container, since it'd offset all their UI elements which I assume would be appended as children fo the map container.

However I think it might be useful to push the current state of padding to our own CSS classes and DOM elements which we use for nesting NavigationControl and AttributionControl

top,left, right and bottom attributes are always 0, i think we can update them from within Map.

Having Map be the only API entrypoint seems more intuitive to me, with plugins automatically respecting it as well.

Here's a demo of what I mean.

…mmetric-viewport

astojilj · 2019-08-20T15:15:02Z

        this._calcMatrices();
    }

+    get padding(): EdgeInsetJSON { return this._edgeInsets.toJSON(); }


@arindam1993 , related to this - what's your opinion about propagating CSS style padding specified to Map's parent HTML container to transform?

… passed in

astojilj

In addition to questions about PaddingOptions and usefulness of padding color shader...
I think it might be useful to add a test that is combining usage of PaddingOptions (Map fitBounds) and camera.setPadding. If we need to have both mechanisms in API , good to document the difference.

astojilj · 2019-08-27T10:44:20Z

+}
+
+export type EdgeInsetLike = EdgeInsets | {top?: number, bottom?: number, right?: number, left?: number} | EdgeInsetJSON;
+export type EdgeInsetJSON = {top: number, bottom: number, right: number, left: number}


Why not using PaddingOptions?

I agree that padding might be a better name here since that is what is exposed.

astojilj · 2019-08-27T11:00:35Z


 export default drawDebug;

+const topColor = new Color(1, 0, 0, 0.4);


Does draw_debug.js go to production code?
I'm not sure how useful is to including additional shader and this code for rendering different colors or padding values.
For manual debugging, propose to add additional debug/***.html page (doesn't go to production code AFAIK) with overlays (in the way the customers are supposed to use it)...

manually test the debug page

and in that that page would be used for this step.

Please doublecheck my opinion with other reviewers - I might be missing something...

draw_debug does go into the final bundle, and we do publish docs for showTileBoundaries and showCollisionBoxes. My idea was to make a similar thing for padding as well.
https://docs.mapbox.com/mapbox-gl-js/api/#map#showtileboundaries
I'm also using the in-gl rendering of the padding as a render-test, similar to tile boundaries and collision box debugging.

And regarding the debug page, I agree, I can add one.

Does draw_debug.js go to production code?

Having this as part of the built-in debug shaders allows easily debugging issues when building/updating work in this area. I can see it being very useful for debugging flyTo animations for example.

That said, this PR adds a total of +4.13 kB to the gl-js bundle. Is there a simpler debug option that can be built?

Thanks for adding a debug view. They can be pretty useful for understanding things. If the size and complexity of the shader another approach could be to use a gl scissor and clear to draw a line at each edge.

gl.enable(gl.SCISSOR_TEST); gl.scissor(x, y, width, height); context.clear({ color }); gl.disable(gl.SCISSOR_TEST);

^^ I did this, it is much simpler though it only saved half a kb of bundle size.

asheemmamoowala

Do custom layers have access to the modified transform that allows correctly rendering when padding values are set?

ansis

Thanks for porting this!

I left a couple comments in places I think could be a bit clearer. The only user facing issue I found was the interpolation one

ansis · 2019-08-27T19:26:12Z

+        if (target.left != null) this.left = number(this.left, target.left, t);
+        if (target.right != null) this.right = number(this.right, target.right, t);
+
+        return this;


This interpolates the padding between the previous frame's value and the target instead of from the starting value to the target. Switching to the more functional approach used for other interpolation could be a good idea.

ansis · 2019-08-30T19:37:36Z

+        const x = Math.min(this.left, width) + 0.5 * (width - totalXInset);
+        const y = Math.min(this.top, height) + 0.5 * (height - totalYInset);
+
+        return new Point(x, y);


Would this be simpler as this?

const x = (width + this.left - this.right) / 2; const y = (height + this.top - this.bottom) / 2;

The main difference would be that in the case where left + right > width it would take equal amounts off of both sides rather than only compromising on the right side. Not sure which is preferable.

I think the simpler one you suggested is better!
Do you think we should throw an error/warning when the overflow happens?

ansis · 2019-08-30T19:39:00Z

+}
+
+export type EdgeInsetLike = EdgeInsets | {top?: number, bottom?: number, right?: number, left?: number} | EdgeInsetJSON;
+export type EdgeInsetJSON = {top: number, bottom: number, right: number, left: number}


I agree that padding might be a better name here since that is what is exposed.

ansis · 2019-08-30T19:50:40Z

+        //Apply center of perspective offset
+        m[8] = -offset.x * 2 / this.width;
+        m[9] = offset.y * 2 / this.height;
+


Can this be done using the methods provided by mat4 (translate, multiply, etc) rather than directly editing the matrix directly? It took me a while to understand the math here

the only api method that allows that I think can work is the asymmetric view frustum method but that takes 4 different fov's as input, and back calculating that seemed more janky to me.

ansis · 2019-08-30T20:57:55Z

        }

+        if (paddingChanged) {
+            this.fire(new Event('paddingstart', eventData))


What do you think these events might be used for? seems reasonable but I can't think of any clear examples right now

Some kind of UI state synchronization?
for example, if you want a floating sidebar to be extended out exactly as much as the map's current value of padding, you can use the events to synchronize that state.

Given the use case, I think it's an unlikely case — you set the map padding because of a floating sidebar, so the state goes in the UI -> map direction, I can't think of use cases with the other way around.

tru, removed them for simplicity!

ansis · 2019-08-30T21:16:41Z


 export default drawDebug;

+const topColor = new Color(1, 0, 0, 0.4);


Thanks for adding a debug view. They can be pretty useful for understanding things. If the size and complexity of the shader another approach could be to use a gl scissor and clear to draw a line at each edge.

gl.enable(gl.SCISSOR_TEST); gl.scissor(x, y, width, height); context.clear({ color }); gl.disable(gl.SCISSOR_TEST);

…mmetric-viewport # Conflicts: # src/geo/transform.js # src/render/draw_debug.js # src/render/program/debug_program.js # src/render/program/program_uniforms.js

arindam1993 · 2020-02-06T00:27:33Z

@asheemmamoowala @astojilj @ansis , I think I've addressed your previous comments, could you all please take another pass at this, I think this is ready to go now

arindam1993 · 2020-02-06T00:28:16Z

Also, @chloekraw its all called padding now

mourner

Just noting that +1kb to the min-zipped bundle seems like a big bump for the seemingly small change — anything we could do to simplify the code? Pointed out a few options.

mourner · 2020-02-06T13:48:03Z

        }

+        if (paddingChanged) {
+            this.fire(new Event('paddingstart', eventData))


Given the use case, I think it's an unlikely case — you set the map padding because of a floating sidebar, so the state goes in the UI -> map direction, I can't think of use cases with the other way around.

mourner · 2020-02-06T13:49:39Z

+    context.clear({color});
+    gl.disable(gl.SCISSOR_TEST);
+}
+


Do you anticipate this debug code being used often, or did it already serve its purpose since the asymmetric viewport implementation is complete? Just noting an opportunity to potentially cut this for smaller bundle size impact.

Not super often, no actually, but it seems like an useful tool to have for render tests, examples and debug pages. I could make it so it is only available in the dev build, but it will break our usual convention.

I've also added some new render tests in the newest commit that represents the distortion caused by asymmetric viewport much better.
https://github.com/mapbox/mapbox-gl-js/pull/8638/files#diff-633f8e084d5e1ba0bb38d2c145e1b771

mourner · 2020-02-06T13:51:12Z

+        if (target.top != null && start.top != null) this.top = number(start.top, target.top, t);
+        if (target.bottom != null && start.bottom != null) this.bottom = number(start.bottom, target.bottom, t);
+        if (target.left != null && start.left != null) this.left = number(start.left, target.left, t);
+        if (target.right != null && start.right != null) this.right = number(start.right, target.right, t);


This looks a little involved — can we get rid of the ifs by defaulting them to 0?

this logic ensures that the if the key is not present the value does not change change from the existing value logic for adding padding. Defaulting to 0 would remove the padding. and having it here ensures that that logic does not have to be implemented in every public interface function that accepts padding as input.

mourner · 2020-02-06T13:53:33Z

+export type PaddingOptions = {top: ?number, bottom: ?number, right: ?number, left: ?number};
+export type SerializedPadding = {top: number, bottom: number, right: number, left: number};
+
+export default EdgeInsets;


If we simplify this class somewhat, could it be small enough to the point it's simpler to have this state as a part of transform rather than a separate class?

It could be, but this seems easier to write unit tests for, and handles a lot of the validation and updating logic without adding additional private methods to the transform class.

Co-Authored-By: Vladimir Agafonkin <agafonkin@gmail.com>

…port with a crosshatch road pattern

…ring

arindam1993 · 2020-02-13T01:38:50Z

Added some fresh render tests that visualize the effect of the distortion better.

arindam1993 · 2020-02-13T20:17:52Z

YAY merging!!!! thanks everyone for the detailed review! 🙇

Arindam Bose added 8 commits August 9, 2019 14:42

add edge insets class

a9cc2f9

add unit tests for EdgeInsets

537562c

add EdgeInsets docs comments

1ce9ea1

WIP tests for rendering padding

e79edaf

working padding visualization

03ac661

working initial transition

e01bafa

working centerPoint refactor

c8bb865

fix bugs with camera transition functions and padding logic

8bb8460

arindam1993 requested review from ansis and astojilj August 15, 2019 01:35

Arindam Bose added 3 commits August 15, 2019 14:24

account for pixelRatio in showPadding

649e1d4

Add synchrounous getPadding and setPadding methods to Map

aa7fd47

fix broken import

7a479e3

astojilj reviewed Aug 19, 2019

View reviewed changes

Arindam Bose added 2 commits August 19, 2019 17:12

add render tests for visualizing padding and fix camera unit test

bbba3ee

Merge branch 'master' of github.com:mapbox/mapbox-gl-js into feat/asy…

1cdf37f

…mmetric-viewport

astojilj reviewed Aug 20, 2019

View reviewed changes

arindam1993 mentioned this pull request Aug 21, 2019

Ensure controls respect padding #8670

Closed

7 tasks

add unit test that ensures padding doesnt change if no new values are…

64522f5

… passed in

astojilj reviewed Aug 27, 2019

View reviewed changes

arindam1993 self-assigned this Aug 27, 2019

asheemmamoowala reviewed Aug 27, 2019

View reviewed changes

asheemmamoowala changed the title ~~Port asymmetric viewport rendering to gl-js~~ [dnm] Port asymmetric viewport rendering to gl-js Aug 29, 2019

ansis reviewed Aug 30, 2019

View reviewed changes

Arindam Bose added 4 commits September 9, 2019 14:33

Merge branch 'master' of github.com:mapbox/mapbox-gl-js into feat/asy…

d0080a4

…mmetric-viewport # Conflicts: # src/geo/transform.js # src/render/draw_debug.js # src/render/program/debug_program.js # src/render/program/program_uniforms.js

fix linter issues

3833d3e

fix more linter issues

d10d082

Fix edgeInset interpolation issue

29bd59e

mourner mentioned this pull request Sep 25, 2019

Need padding in flyto/jumpTo/easeTo #8794

Closed

Arindam Bose added 4 commits February 4, 2020 17:38

Account for off-axis padding in fitBounds and related functions

b98ffea

Use glClear for drawing debug padding

39f4288

Delete unused ss_rect shader

44a2654

Fix missing imports

00de0c6

arindam1993 mentioned this pull request Feb 6, 2020

Add a padding option to map.getBounds #9265

Open

mourner reviewed Feb 6, 2020

View reviewed changes

Arindam Bose and others added 6 commits February 12, 2020 14:12

Remove duplicated centerPoint code

372099a

Co-Authored-By: Vladimir Agafonkin <agafonkin@gmail.com>

Remove padding events from camera

e0871f7

Remove unused SerializedPadding type

354a555

Update render tests to represent distortion caused by asymmetric view…

fbb26fa

…port with a crosshatch road pattern

Fix lint error caused by unused variables

b7e8e6c

Update render tests to fix precision errors with 0 padding line rende…

2d0e390

…ring

mourner approved these changes Feb 13, 2020

View reviewed changes

arindam1993 merged commit 0412fdb into master Feb 13, 2020

arindam1993 deleted the feat/asymmetric-viewport branch February 13, 2020 20:19

This was referenced Feb 13, 2020

Add a global "padding" option #4268

Closed

Port asymmetric viewport PR#14664 from gl-native to gl-js #8328

Closed

waissbluth mentioned this pull request Mar 29, 2020

map.fitBounds(...) behaves differently in v 1.9.0 #9477

Closed

harto mentioned this pull request Apr 23, 2020

Permit padding in transitions visgl/react-map-gl#1085

Closed

candidia mentioned this pull request Apr 25, 2020

flyTo options doesn't support padding alex3165/react-mapbox-gl#859

Open

arindam1993 mentioned this pull request Apr 29, 2020

Calling map.easeTo() after map.fitBounds() with a padding does not preserve the padding #9643

Closed

tmcgann mentioned this pull request Apr 26, 2022

Maxbounds pixel buffer setting? #5997

Closed

leap95 mentioned this pull request Apr 17, 2024

map.flyTo({ center: someCenter, padding: somePadding }) ignores padding #13152

Closed


		export default drawDebug;

		const topColor = new Color(1, 0, 0, 0.4);

Uh oh!

Conversation

arindam1993 commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

API:

Testing:

Demo:

Launch Checklist

Uh oh!

astojilj commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astojilj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chloekraw Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astojilj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arindam1993 Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asheemmamoowala left a comment

Choose a reason for hiding this comment

Uh oh!

ansis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arindam1993 commented Aug 15, 2019 •

edited

Loading

astojilj commented Aug 19, 2019 •

edited

Loading

chloekraw Aug 19, 2019 •

edited

Loading

arindam1993 Aug 27, 2019 •

edited

Loading

arindam1993 commented Feb 6, 2020 •

edited

Loading