Bug 220732 - Complete WebXRRigidTransform implementation
Summary: Complete WebXRRigidTransform implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebXR (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Imanol Fernandez
URL:
Keywords: InRadar
Depends on:
Blocks: 208988
  Show dependency treegraph
 
Reported: 2021-01-19 08:31 PST by Imanol Fernandez
Modified: 2021-01-27 03:04 PST (History)
9 users (show)

See Also:


Attachments
Patch (14.24 KB, patch)
2021-01-22 04:48 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (14.71 KB, patch)
2021-01-22 06:16 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (14.61 KB, patch)
2021-01-25 02:25 PST, Imanol Fernandez
svillar: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.36 KB, patch)
2021-01-25 04:29 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (14.49 KB, patch)
2021-01-25 04:55 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff
Patch (14.81 KB, patch)
2021-01-26 01:32 PST, Imanol Fernandez
svillar: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (14.71 KB, patch)
2021-01-27 02:10 PST, Imanol Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Imanol Fernandez 2021-01-19 08:31:53 PST
- Implement inverse()
- Expose raw matrix to be used in XRRigitTransform and XRPose math operations
- Correctly reuse the Float32Array matrixData
Comment 1 Imanol Fernandez 2021-01-22 04:48:31 PST
Created attachment 418139 [details]
Patch
Comment 2 Sergio Villar Senin 2021-01-22 05:45:43 PST
Comment on attachment 418139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418139&action=review

Looking pretty good, just one question...

> Source/WebCore/Modules/webxr/WebXRRigidTransform.cpp:153
> +    m_inverse->m_inverse = this;

Aren't we creating a reference loop here?

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:585
> +// in TransformationMatrix::recompose4().

Nit: you can put everything in a single line
Comment 3 Imanol Fernandez 2021-01-22 06:16:42 PST
Created attachment 418143 [details]
Patch
Comment 4 Sergio Villar Senin 2021-01-25 01:32:47 PST
Comment on attachment 418143 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418143&action=review

> Source/WebCore/Modules/webxr/WebXRRigidTransform.cpp:131
> +    // If the operation IsDetachedBuffer on internal matrix is false, return transformâs internal matrix.

Nit: I think the code is self explanatory here, maybe we don't need the comment.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:584
> +// TODO: Once https://bugs.webkit.org/show_bug.cgi?id=220856 is addressed we can reuse this function in TransformationMatrix::recompose4().

Nit: in WebKit we normally use FIXME instead of TODO
Comment 5 Imanol Fernandez 2021-01-25 02:25:23 PST
Created attachment 418267 [details]
Patch
Comment 6 Imanol Fernandez 2021-01-25 02:26:08 PST
Thanks for the review, I fixed the nits in the last patch
Comment 7 EWS 2021-01-25 03:29:05 PST
ChangeLog entry in Source/WebCore/ChangeLog is not at the top of the file.
Comment 8 Imanol Fernandez 2021-01-25 04:29:08 PST
Created attachment 418273 [details]
Patch
Comment 9 Imanol Fernandez 2021-01-25 04:55:42 PST
Created attachment 418277 [details]
Patch
Comment 10 Imanol Fernandez 2021-01-26 01:32:36 PST
Created attachment 418379 [details]
Patch
Comment 11 Radar WebKit Bug Importer 2021-01-26 08:32:25 PST
<rdar://problem/73617302>
Comment 12 EWS 2021-01-26 12:26:33 PST
Downloading mechanize-0.4.5...
Installing mechanize-0.4.5...
Installed mechanize-0.4.5!
Sergio Villar found in /Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 13 Imanol Fernandez 2021-01-27 02:10:23 PST
Created attachment 418519 [details]
Patch for landing
Comment 14 EWS 2021-01-27 03:04:22 PST
Committed r271941: <https://trac.webkit.org/changeset/271941>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418519 [details].