Fix inconsistent transform flattening logic

ASSIGNED
Assigned to

Status

()

enhancement
P3
normal
ASSIGNED
3 months ago
16 hours ago

People

(Reporter: gw, Assigned: kvark)

Tracking

(Regressed 1 bug, {leave-open})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [wr-q2][wr-may])

Attachments

(7 attachments, 1 obsolete attachment)

Reporter

Description

3 months ago

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.

Reporter

Comment 1

3 months ago

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

Flags: needinfo?(dmalyshau)
Assignee

Comment 2

3 months ago

On it!

Assignee: nobody → dmalyshau
Status: NEW → ASSIGNED
Flags: needinfo?(dmalyshau)
Assignee

Comment 3

2 months ago

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

Assignee

Comment 4

2 months ago

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

Assignee

Updated

2 months ago
Keywords: leave-open
Assignee

Comment 5

2 months ago

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.

Assignee

Comment 6

2 months ago

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.

Assignee

Comment 7

2 months ago

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

Comment 8

2 months ago
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/662d2392cb01
Enable env_logger by default in Wrench r=gw

Comment 9

2 months ago
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46ad1441e7f2
Strongly typed vertex data stores in WR r=gw

Comment 10

2 months ago
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c154ee9861ab
Refactor get_relative_transform to not return an option r=gw
Assignee

Comment 11

2 months ago

needed to avoid duplicate dependencies

Priority: -- → P3
Whiteboard: [wr-q2][wr-may]
Assignee

Comment 14

18 days ago

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...

Assignee

Comment 15

17 days ago

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.

Assignee

Comment 16

17 days ago

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

Attachment #9063273 - Attachment is obsolete: true

Comment 17

16 days ago
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a44b50a07cf
Refactor WR fast transform, use when returning relative transforms r=gw
Assignee

Comment 19

14 days ago

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

14 days ago
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73254a69497b
Repace WR RelativeTransform with CoordinateSpaceMapping, improve flattening semantics. r=gw

Comment 23

a day ago
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a56b157399e1
WR cache world transform on coordinate systems r=gw
You need to log in before you can comment on or make changes to this bug.