Fix inconsistent transform flattening logic
Categories
(Core :: Graphics: WebRender, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: gw, Assigned: kvark)
References
(Regressed 2 open bugs)
Details
(Whiteboard: [wr-q2][wr-may])
Attachments
(8 files, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
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•6 years ago
|
||
Dzmitry, you said you may have cycles to work on this?
Assignee | ||
Comment 2•6 years ago
|
||
On it!
Assignee | ||
Comment 3•6 years ago
|
||
I'm having a hard time rewriting this. The gist of complexity is the following:
- flattening is forcing the transform to produce Z=0
- therefore, transforms are no longer fully invertible across the flatten spots
- 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•6 years 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•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
Env logger is useful for:
- 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.
- 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•6 years 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•6 years ago
|
||
Just a refactor without any semantical changes.
Makes it for cleaner client code. Also prepares for caching of the world transforms.
Comment 10•6 years ago
|
||
Assignee | ||
Comment 11•6 years ago
|
||
needed to avoid duplicate dependencies
Comment 12•6 years ago
|
||
Comment 13•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 14•5 years 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•5 years 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•5 years ago
|
||
Do not zero out the transformed Z. Instead, pass it through unchanged.
Updated•5 years ago
|
Comment 17•5 years ago
|
||
Comment 18•5 years ago
|
||
bugherder |
Assignee | ||
Comment 19•5 years 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•5 years ago
|
||
Comment 21•5 years ago
|
||
bugherder |
Assignee | ||
Comment 22•5 years ago
|
||
Comment 23•5 years ago
|
||
Comment 24•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 25•5 years ago
|
||
This is the last big step towards consistent flattening of transformations.
It includes removing the old "project_to_2d" method from the utils.
Updated•5 years ago
|
Comment 26•5 years ago
|
||
Comment 27•5 years ago
|
||
bugherder |
Description
•