Refactor usage of implicit parameter to an explicit one when rendering #5020

ibesora · 2024-11-08T17:34:17Z

This PR aims to refactor our codebase a little bit in order to move away of what, IMHO, are code smells.

First, some context.
The Painter object mostly contains properties that describe how things are going to be rendered via the underlying rendering API (i.e. vertex and index buffers, stencil buffer definitions, depth range, etc.) but it also contains the Style object which in itself describes how things should look like.
draw_* functions (i.e. draw_background) get both the Painter object and StyleLayer object, which is basically the subset of the Style that we need to render a given layer.

There are various places in the draw_* functions where we use painter.style to get information about the current rendering mode:

We use painter.style.map.terrain to know if we are rendering with terrain
We use painter.style.projection.name to know if we are rendering using globe or mercator projections

and change the drawing behaviour according to that. Since those properties are deep into the Painter object, you might get two similar-looking draw_* calls with very different results.

This PR replaces checks for terrain rendering via the painter.style property with an extra renderLayer parameter that tells a given draw_* function if it's being rendered to a texture.

A follow-up idea would be to remove style entirely from the Painter object and add all the properties that are needed for the various draw_* functions into the Painter object itself.

Cc @birkskyum since we talked about this.

Launch Checklist

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Post benchmark scores.
Add an entry to CHANGELOG.md under the ## main section.

codecov-commenter · 2024-11-08T17:36:57Z

Codecov Report

Attention: Patch coverage is 98.58156% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.43%. Comparing base (8555091) to head (482a8cd).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/render/draw_line.ts	80.00%	0 Missing and 1 partial ⚠️
src/render/render_to_texture.ts	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5020      +/-   ##
==========================================
- Coverage   89.25%   88.43%   -0.82%     
==========================================
  Files         269      269              
  Lines       38286    38310      +24     
  Branches     2347     2478     +131     
==========================================
- Hits        34172    33880     -292     
- Misses       3117     3393     +276     
- Partials      997     1037      +40

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

birkskyum · 2024-11-08T17:47:28Z

Thanks for cleaning up - I think this is a good path.

To make it even more explicit what happens, can we maybe flip the bools, so that they're apply (or enable) instead of ignore? My thinking is that it's easier to understand what happens if we turn on logic, and it opens for later on just passing in the transform we want to run, instead of focusing on disabling the ones that we don't want to run. There's effectively a lot of double-negation in the current design.

So effectively this:

this.getProjectionData({
  ignoreGlobeMatrix: true,
//ignoreTerrainMatrix: false  hidden default value
});

Becomes this:

this.getProjectionData({
//applyGlobeMatrix: false, hidden default value
  applyTerrainMatrix: true
});

So it says what it does, not what it doesn't.

HarelM · 2024-11-08T22:07:18Z

I think this is great!
I would also look in the code and find the places where "this" is being passed down. Solving those places will prevent this behavior I believe where you look deep into objects to find references to other objects.

Since we are in the middle of a breaking change version release, now would be a great time to move objects around and clean some historical bad decisions...

src/render/painter.ts

src/render/draw_heatmap.ts

ibesora · 2024-11-12T11:34:16Z

I've updated this PR with some of your suggestions:

To remove reading globe from the style as suggested here, instead of adding another boolean to all the draw_* functions, I moved those into a type called RenderOptions that will be able to grow with more flags.
I also added type predicate functions to change remove the layer as any casts.

I'll tackle removing style from the Painter object in another PR

HarelM · 2024-11-12T11:34:34Z

src/render/painter.ts

@@ -62,6 +72,11 @@ type PainterOptions = {
    fadeDuration: number;
 };

+export type RenderFlags = {


I think "Options" is used in the code. I would consider renaming this to "RenderOptions".

HarelM · 2024-11-12T11:38:18Z

src/render/painter.ts

-        switch (layer.type) {
-            case 'symbol':
-                drawSymbols(painter, sourceCache, layer as any, coords, this.style.placement.variableOffsets);
+        switch (true) {


I find this odd TBH, maybe a if ...else if... Will be better here? It might be just that I'm not used to it... IDK.

Yeah, to be fair it was a little bit weird. Changed it to an if...else block and it feels better.

HarelM · 2024-11-12T11:40:55Z

src/render/draw_background.ts

 import type {SourceCache} from '../source/source_cache';
 import type {BackgroundStyleLayer} from '../style/style_layer/background_style_layer';
 import {OverscaledTileID} from '../source/tile_id';
 import {coveringTiles} from '../geo/projection/covering_tiles';

-export function drawBackground(painter: Painter, sourceCache: SourceCache, layer: BackgroundStyleLayer, coords: Array<OverscaledTileID>, renderFlags: RenderFlags) {
+export function drawBackground(painter: Painter, sourceCache: SourceCache, layer: BackgroundStyleLayer, coords: Array<OverscaledTileID>, renderFlags: RenderOptions) {


Don't forget to change the parameter name as well - renderOptions...

HarelM · 2024-11-12T11:41:29Z

LGTM, thanks!

src/geo/projection/globe_transform.test.ts

birkskyum · 2024-11-12T13:06:54Z

src/render/draw_background.ts

@@ -47,7 +47,7 @@ export function drawBackground(painter: Painter, sourceCache: SourceCache, layer
    for (const tileID of tileIDs) {
        const projectionData = transform.getProjectionData({
            overscaledTileID: tileID,
-            ignoreGlobeMatrix: globeWithTerrain
+            applyGlobeMatrix: !isRenderingToTexture


is an applyTerrainMatrix: true missing here.

birkskyum · 2024-11-12T13:08:25Z

src/render/draw_circle.ts

@@ -80,7 +81,7 @@ export function drawCircles(painter: Painter, sourceCache: SourceCache, layer: C
        const terrainData = painter.style.map.terrain && painter.style.map.terrain.getTerrainData(coord);
        const uniformValues = circleUniformValues(painter, tile, layer, translateForUniforms, radiusCorrectionFactor);

-        const projectionData = transform.getProjectionData({overscaledTileID: coord});
+        const projectionData = transform.getProjectionData({overscaledTileID: coord, applyGlobeMatrix: !isRenderingToTexture});


Is an applyTerrainMatrix missing here too?

birkskyum · 2024-11-12T13:14:12Z

src/render/draw_fill.ts

        const projectionData = transform.getProjectionData({
            overscaledTileID: coord,
-            ignoreGlobeMatrix: globeWithTerrain
+            applyGlobeMatrix: !isRenderingToTexture


Here also, I'd expect applyTerrainMatrix: true

birkskyum · 2024-11-12T13:14:39Z

src/render/draw_hillshade.ts

        const projectionData = transform.getProjectionData({
            overscaledTileID: coord,
            aligned: align,
-            ignoreGlobeMatrix: globeWithTerrain
+            applyGlobeMatrix: !isRenderingToTexture


here too, I expected an additional applyTerrainMatrix: true

HarelM reviewed Nov 8, 2024

View reviewed changes

src/render/painter.ts Outdated Show resolved Hide resolved

HarelM approved these changes Nov 8, 2024

View reviewed changes

birkskyum reviewed Nov 8, 2024

View reviewed changes

src/render/draw_heatmap.ts Outdated Show resolved Hide resolved

ibesora added 6 commits November 12, 2024 10:28

Use explicit parameter instead of implicit one

376a0b0

Add parameter to all draw functions

32c1a37

Add predicates

59b60ad

Don't use default parameter

daabb24

Move to apply instead of ignore

981a7e6

Move booleans to type

0abf8a6

ibesora force-pushed the ib/improve-rtt-path branch from 863168e to 0abf8a6 Compare November 12, 2024 11:27

HarelM reviewed Nov 12, 2024

View reviewed changes

ibesora added 2 commits November 12, 2024 12:34

Fixes

fcd056c

Rename RenderFlags to RenderOptions

a0ad0d4

HarelM reviewed Nov 12, 2024

View reviewed changes

birkskyum reviewed Nov 12, 2024

View reviewed changes

src/geo/projection/globe_transform.test.ts Show resolved Hide resolved

ibesora added 3 commits November 12, 2024 13:31

Rename parameter and change switch

0add9a0

More fixes

0abe453

More renaming

482a8cd

birkskyum reviewed Nov 12, 2024

View reviewed changes

birkskyum mentioned this pull request Nov 13, 2024

Projection expressions #5040

Draft

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor usage of implicit parameter to an explicit one when rendering #5020

Refactor usage of implicit parameter to an explicit one when rendering #5020

ibesora commented Nov 8, 2024

codecov-commenter commented Nov 8, 2024 •

edited

Loading

birkskyum commented Nov 8, 2024 •

edited

Loading

HarelM commented Nov 8, 2024

ibesora commented Nov 12, 2024 •

edited

Loading

HarelM Nov 12, 2024

HarelM Nov 12, 2024

ibesora Nov 12, 2024

HarelM Nov 12, 2024

HarelM commented Nov 12, 2024

birkskyum Nov 12, 2024 •

edited

Loading

birkskyum Nov 12, 2024 •

edited

Loading

birkskyum Nov 12, 2024

birkskyum Nov 12, 2024 •

edited

Loading

Refactor usage of implicit parameter to an explicit one when rendering #5020

Are you sure you want to change the base?

Refactor usage of implicit parameter to an explicit one when rendering #5020

Conversation

ibesora commented Nov 8, 2024

Launch Checklist

codecov-commenter commented Nov 8, 2024 • edited Loading

Codecov Report

birkskyum commented Nov 8, 2024 • edited Loading

HarelM commented Nov 8, 2024

ibesora commented Nov 12, 2024 • edited Loading

HarelM Nov 12, 2024

Choose a reason for hiding this comment

HarelM Nov 12, 2024

Choose a reason for hiding this comment

ibesora Nov 12, 2024

Choose a reason for hiding this comment

HarelM Nov 12, 2024

Choose a reason for hiding this comment

HarelM commented Nov 12, 2024

birkskyum Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

birkskyum Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

birkskyum Nov 12, 2024

Choose a reason for hiding this comment

birkskyum Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Nov 8, 2024 •

edited

Loading

birkskyum commented Nov 8, 2024 •

edited

Loading

ibesora commented Nov 12, 2024 •

edited

Loading

birkskyum Nov 12, 2024 •

edited

Loading

birkskyum Nov 12, 2024 •

edited

Loading

birkskyum Nov 12, 2024 •

edited

Loading