1532174 - Fix inconsistent transform flattening logic

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P3)

Description

[Glenn Watson [:gw]]

The code that derives world_content_transform has different logic for transform flattening than the code in get_relative_transform.

Ideally, we could probably remove the code path that calculates world_content_transform and derive this directly from the get_relative_transform code path.

There are a small number of places in the existing code (perhaps 4 or 5) that read the current value of world_content_transform which would also need to be updated.

Comment 1

[Glenn Watson [:gw]]

Dzmitry, you said you may have cycles to work on this?

Comment 2

[Dzmitry Malyshau [:kvark]]

On it!

Comment 3

[Dzmitry Malyshau [:kvark]]

I'm having a hard time rewriting this. The gist of complexity is the following:

1. flattening is forcing the transform to produce Z=0
2. therefore, transforms are no longer fully invertible across the flatten spots
3. removing the inverse transform from our palette entirely is almost feasible, at least within Rust code, but then faces the use in write_clip_tile_vertex which is not trivial to remove without a deeper redesign on how our clips work...

The WIP code can be seen at - https://github.com/kvark/webrender/tree/rd-repro-buffer-data

Comment 4

[Dzmitry Malyshau [:kvark]]

Forgot to mention that preserve-3d uses the inverse transforms extensively, but this may be addressed by adopting (and further evolving) the patch Glenn proposed earlier - https://phabricator.services.mozilla.com/D21333

Comment 5

[Dzmitry Malyshau [:kvark]]

Attachment: Enable env_logger by default in Wrench

Env logger is useful for:

1. seeing error! generated by the code (by default). Sometimes it can give us hints on what is wrong with the capture, or a test right away.
2. seeing processed shader compile/link errors, which is very useful when changing them

Given that wrench is the main debugging tool at the moment, enabling the logging output by default should increase our productivity, at the cost of slightly longer build times.

Comment 6

[Dzmitry Malyshau [:kvark]]

Attachment: Strongly typed vertex data stores in WR

Establishes a clear link of TransformData with the shader.
Associates a vertex data type permanently, which makes the code cleaner and allow finding the relevant bits quickly with search.

Comment 7

[Dzmitry Malyshau [:kvark]]

Attachment: Refactor get_relative_transform to not return an option

Just a refactor without any semantical changes.
Makes it for cleaner client code. Also prepares for caching of the world transforms.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/662d2392cb01
Enable env_logger by default in Wrench r=gw

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46ad1441e7f2
Strongly typed vertex data stores in WR r=gw

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c154ee9861ab
Refactor get_relative_transform to not return an option r=gw

Comment 11

[Dzmitry Malyshau [:kvark]]

Attachment: Roll back env_logger to 0.5

needed to avoid duplicate dependencies

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c83d53042c6
Roll back env_logger to 0.5 r=gw

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/662d2392cb01
https://hg.mozilla.org/mozilla-central/rev/46ad1441e7f2
https://hg.mozilla.org/mozilla-central/rev/c154ee9861ab
https://hg.mozilla.org/mozilla-central/rev/2c83d53042c6

Comment 14

[Dzmitry Malyshau [:kvark]]

I've been looking at this a bit more. We previously assumed that one of the important blocks of the solution would be moving plane splitting to happen in the space of the preserve3d root. Now I start to understand that this is going to be much less pleasant than we'd hope.

Ordering itself is fine to do in any space. However, we are currently doing the frustum clipping of the planes in order to avoid any of the resulting points/planes to fall behind the W-positive hemisphere in homogeneous space. We do this before registering the planes for splitting and ordering. In order to change the space, we'd need to consider the frustum in a different space, which in turn can be perspective transformed and such, which complicate things quite a bit...

Comment 15

[Dzmitry Malyshau [:kvark]]

Attachment: Refactor WR fast transform, use when returning relative transforms

Instead of converting from the scale-offset to the transform right away in
get_relative_transform, we only do it if there is a jump between coordinate spaces.

Comment 16

[Dzmitry Malyshau [:kvark]]

Attachment: WR make transform flattening inversible

Do not zero out the transformed Z. Instead, pass it through unchanged.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a44b50a07cf
Refactor WR fast transform, use when returning relative transforms r=gw

Comment 18

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/5a44b50a07cf

Comment 19

[Dzmitry Malyshau [:kvark]]

Attachment: Repace WR RelativeTransform with CoordinateSpaceMapping, improve flattening semantics.

This change makes get_relative_transform() to no longer rely on any flattening done before in the pipeline.
This makes it correct is some of the cases we failed previously (see ini files removed).
It now does flattening on every flat coordinate system it passes through, and it's used for SpaceMapper.
The old RelativeTransform is now replaced with CoordinateSpaceMapping, which reduces the zoo of our types :)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73254a69497b
Repace WR RelativeTransform with CoordinateSpaceMapping, improve flattening semantics. r=gw

Comment 21

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/73254a69497b

Comment 22

[Dzmitry Malyshau [:kvark]]

Attachment: WR cache world transform on coordinate systems

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a56b157399e1
WR cache world transform on coordinate systems r=gw

Comment 24

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/a56b157399e1

Comment 25

[Dzmitry Malyshau [:kvark]]

Attachment: WR remove the world transformations from SpatialNode

This is the last big step towards consistent flattening of transformations.
It includes removing the old "project_to_2d" method from the utils.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f55fc1509616
WR remove the world transformations from SpatialNode r=gw

Comment 27

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/f55fc1509616