Closed Bug 1532174 Opened 1 year ago Closed 1 year ago

Fix inconsistent transform flattening logic

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: gw, Assigned: kvark)

References

(Regressed 1 open bug)

Details

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

Attachments

(8 files, 1 obsolete file)

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.

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

Flags: needinfo?(dmalyshau)

On it!

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

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

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

Keywords: leave-open

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.

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.

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

Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/662d2392cb01
Enable env_logger by default in Wrench r=gw
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46ad1441e7f2
Strongly typed vertex data stores in WR r=gw
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c154ee9861ab
Refactor get_relative_transform to not return an option r=gw

needed to avoid duplicate dependencies

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

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

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.

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

Attachment #9063273 - Attachment is obsolete: true
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a44b50a07cf
Refactor WR fast transform, use when returning relative transforms r=gw

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 :)

Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73254a69497b
Repace WR RelativeTransform with CoordinateSpaceMapping, improve flattening semantics. r=gw
Regressions: 1551520
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a56b157399e1
WR cache world transform on coordinate systems r=gw
Keywords: leave-open

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

Attachment #9067442 - Attachment description: [WIP] WR remove the world transformations from SpatialNode → WR remove the world transformations from SpatialNode
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f55fc1509616
WR remove the world transformations from SpatialNode r=gw
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.