Support WebXR Meshing [chromium/src : main]
Alexander Cooper (Gerrit)
Alexander Cooper added 37 comments![]( "Open in Gerrit")
Mostly high level review. I didn't go into the details of any of the core algorithms in blink or OpenXR
mojom::XRSessionFeature::MESH_DETECTION,Alexander CooperI don't think this is actually supported here?
Oh, and at a bare minimum, it should be appended like the WEBGPU feature is below.
std::set<uint64_t> accumulated_mesh_ids_;And absl::flat_hash_set as well
std::unordered_map<XrUuid, uint64_t, XrUuidHash, XrUuidEqual> uuid_to_id_;Guidance is to use absl::flat_hash_map for better performance.
https://chromium.googlesource.com/chromium/src/+/main/base/containers/README.md
XrSceneMeshingTrackerANDROID mesh_tracker() const { return mesh_tracker_; }Seems odd this needs to be public?
return data64[0] ^ (data64[1] + 0x9e3779b9 + (data64[0] << 6) + (data64[0] >> 2));What is this? Why can the XrUUID not just be hashed as-is? Why do we need sorting? XrUUIDMSFT is able to do this as a simple strong alias to an ID type, though I think maybe we should use the id generator pattern with a map, I'm wondering if some default hashing is fine here.
std::copy_n(data_span.begin(), 8, reinterpret_cast<uint8_t*>(&data64[0]));I *think* this is a banned construct. Byte_span_from_ref is better: https://source.chromium.org/chromium/chromium/src/+/main:base/containers/span.h;drc=ded9cdb6eb6253321e19b5bfb01d6c8d57a7aca5;l=1600 (or just as_byte_span). I think there's some concerns with this as well though;
std::string GetSemanticLabelString(XrSceneMeshSemanticLabelANDROID label) {Should be able to re-use: https://source.chromium.org/chromium/chromium/src/+/main:device/vr/public/mojom/vr_service.mojom;drc=0a156043e74e026eb06dbc58a0501c76bc7bc839;l=501
Strings can be resolved in blink.
LOG(WARNING) << "xrCreateSceneMeshSnapshotANDROID failed with result="LOG (vs DVLOG or DLOG) should be used VERY sparingly due to the fact that it bloats the production code.
#ifndef XR_NO_PROTOTYPES
#ifdef XR_EXTENSION_PROTOTYPES
XRAPI_ATTR XrResult XRAPI_CALL xrEnumerateSupportedSemanticLabelSetsANDROID(
XrInstance instance,
XrSystemId systemId,
uint32_t supportedSemanticLabelSetsInputCapacity,
uint32_t* supportedSemanticLabelSetsOutputCount,
XrSceneMeshSemanticLabelSetANDROID* supportedSemanticLabelSets);
XRAPI_ATTR XrResult XRAPI_CALL xrCreateSceneMeshingTrackerANDROID(
XrSession session,
const XrSceneMeshingTrackerCreateInfoANDROID* createInfo,
XrSceneMeshingTrackerANDROID* tracker);
XRAPI_ATTR XrResult XRAPI_CALL xrDestroySceneMeshingTrackerANDROID(
XrSceneMeshingTrackerANDROID tracker);
XRAPI_ATTR XrResult XRAPI_CALL xrCreateSceneMeshSnapshotANDROID(
XrSceneMeshingTrackerANDROID tracker,
const XrSceneMeshSnapshotCreateInfoANDROID* createInfo,
XrSceneMeshSnapshotCreationResultANDROID* outSnapshotCreationResult);
XRAPI_ATTR XrResult XRAPI_CALL xrDestroySceneMeshSnapshotANDROID(
XrSceneMeshSnapshotANDROID snapshot);
XRAPI_ATTR XrResult XRAPI_CALL xrGetAllSubmeshStatesANDROID(
XrSceneMeshSnapshotANDROID snapshot,
uint32_t submeshStateCapacityInput,
uint32_t* submeshStateCountOutput,
XrSceneSubmeshStateANDROID* submeshStates);
XRAPI_ATTR XrResult XRAPI_CALL xrGetSubmeshDataANDROID(
XrSceneMeshSnapshotANDROID snapshot,
uint32_t submeshDataCount,
XrSceneSubmeshDataANDROID* inoutSubmeshData);
#endif /* XR_EXTENSION_PROTOTYPES */
#endif /* !XR_NO_PROTOTYPES */IIRC, This section can be omitted.
// TODO(mesh-detection): Remove this file once XR_ANDROID_scene_meshing
// is officially added to the OpenXR specification and the Chromium
// third_party/openxr submodule is updated.
//Add this as a block to https://source.chromium.org/chromium/chromium/src/+/main:third_party/openxr/dev/xr_android.h instead.
mojom::XRMeshDetectionDataPtr OpenXrMeshManager::GetDetectedMeshesData(
XrTime frame_time) {
return nullptr;
}
std::optional<XrLocation> OpenXrMeshManager::GetXrLocationFromMesh(
MeshId mesh_id,
const gfx::Transform& mesh_id_from_object) const {
return std::nullopt;
}Just make pure virtual (`= 0` in header).
virtual OpenXrMeshManager* GetMeshManager() = 0;Alexander CooperMy understanding from you was that this didn't strictly need to be integrated with the SceneUnderstandingManager framework? The new version *may* need to be part of it, or at least get the spatial discovery snapshot, but until such time as that's ready, I don't think we should do this part of the architecture yet.
That said, I guess it logically makes sense to be part of the infrastructure, so fine to leave as-is in hindsight.
supported_features_.insert(device::mojom::XRSessionFeature::MESH_DETECTION);Irrelevant if you merge into the plane manager, but should use it's own `::IsSupported` method before being added.
// Required by XR_SPATIAL_CAPABILITY_PLANE_TRACKING_EXTRequired... to be supported?
mesh2d_enabled_ =
base::Contains(plane_tracking_components,
XR_SPATIAL_COMPONENT_TYPE_MESH_2D_EXT);
if (mesh2d_enabled_) {
enabled_components_.insert(XR_SPATIAL_COMPONENT_TYPE_MESH_2D_EXT);
LOG(ERROR) << "Adam: [MeshManager] Mesh2D component is SUPPORTED and will "
"be enabled under PLANE_TRACKING.";
} else {
LOG(ERROR) << "Adam: [MeshManager] Mesh2D component is NOT supported for "
"PLANE_TRACKING on this runtime.";
}To be honest, I think we should just put this on the SpatialPlaneManager since it is associated with the planes. If we want to do any merging later, the more-full mesh manager can be created and used then (and receive a reference to the plane manager). I think it'll simplify a lot of this code. *May* need to rework the enablement/supported feature checks in the spatial manager, but
struct XRMeshData {Security likes full comments describing structs and each member in .mojom files.
string? semantic_label;Use existing SemanticLabel type.
template <>
struct TypeConverter<blink::DOMUint32Array*, blink::Vector<uint32_t>> {
static blink::DOMUint32Array* Convert(const blink::Vector<uint32_t>& input);
};
template <>
struct TypeConverter<blink::HeapVector<blink::Member<blink::DOMFloat32Array>>,
blink::Vector<float>> {
static blink::HeapVector<blink::Member<blink::DOMFloat32Array>> Convert(
const blink::Vector<float>& input);
};I'm not sure we want this, as sometimes we may want to work on these types directly before converting them. Sometimes they are converted to different types, etc. You could add a *helper* maybe (in xr_utils or whatever it's called here in blink), but I don't think a type converter is appropriate here.
#include "third_party/blink/renderer/platform/heap/collection_support/heap_vector.h"Keep blank newline after includes
MeshId id() const;Not part of the web IDL, so should be `Id`
const size_t vertex_count = vertices.size() / 3;Put 3 in a constant.
for (size_t i = 0; i < vertex_count; ++i) {
DOMFloat32Array* vertex_array = DOMFloat32Array::Create(3);
auto span = vertex_array->AsSpan();
span[0] = vertices[i * 3 + 0]; // x
span[1] = vertices[i * 3 + 1]; // y
span[2] = vertices[i * 3 + 2]; // z
result.push_back(NotShared<DOMFloat32Array>(vertex_array));
}```
base::span vertices_span(vertices);
while (vertices_span.size() >= 3) {
DOMFloat32Array* vertex_array = DOMFloat32Array::Create(3);
vertex_array->AsSpan().copy_from(vertices);
vertices = vertices.subspan(3);
}
```
LOG(ERROR) << __func__ << "Adam2: mesh_id=" << idSee Log comments
vertices_ = MakeGarbageCollected<FrozenArray<NotShared<DOMFloat32Array>>>(
ConvertVerticesToFloat32Arrays(mesh_data.vertices));For the record, we just had a big performance hit from depth for something probably less intense than this; though I don't know that there's a way around it.
gfx::Point3F camera_position_{0, 0, 0};
gfx::Vector3dF camera_forward_{0, 0, -1};I'm not seeing this as part of the spec.
bool ShouldShowMeshInView(const XRMesh* mesh,Not sure that this is part of the spec?
std::optional<gfx::Transform> mesh_transform = mesh->MojoFromObject();mesh_from_object still. We try to keep a consistent naming scheme with all transforms, even if they are short lived.
gfx::Point3F mesh_pos(mesh_from_object_position should be the end result here FWIW
gfx::Point3F mesh_pos(
mesh_transform->rc(0, 3),
mesh_transform->rc(1, 3),
mesh_transform->rc(2, 3)Use gfx::Transform::Decompose instead, it shouldn't matter for our transforms, but it's a bad habit to not use: https://source.chromium.org/chromium/chromium/src/+/main:ui/gfx/geometry/transform.h;drc=967c368f01062a9d008fcc301843899a01167bff;l=516
float dx = mesh_pos.x() - camera_pos.x();
float dy = mesh_pos.y() - camera_pos.y();
float dz = mesh_pos.z() - camera_pos.z();
float distance = std::sqrt(dx*dx + dy*dy + dz*dz);Can Vector3Df from subtracting the two points not be used to grab the length and get distance that way?
camera_position_ = camera_position;
camera_forward_ = camera_forward;All this data should be achievable from the viewer pose (mojo_from_viewer)? Which would be the better more accurate way to do this if needed.
gfx::Point3F camera_position(0, 0, 0);
gfx::Vector3dF camera_forward(0, 0, -1);
if (frame_data->render_info && frame_data->render_info->mojo_from_viewer) {
auto mojo_from_viewer = getPoseMatrix(frame_data->render_info->mojo_from_viewer);
if (mojo_from_viewer) {
camera_position = gfx::Point3F(
mojo_from_viewer->rc(0, 3),
mojo_from_viewer->rc(1, 3),
mojo_from_viewer->rc(2, 3)
);
camera_forward = gfx::Vector3dF(
-mojo_from_viewer->rc(0, 2),
-mojo_from_viewer->rc(1, 2),
-mojo_from_viewer->rc(2, 2)
);
camera_forward.GetNormalized(&camera_forward);
}
}See comment about just passing in and using mojo_from_viewer for this directly; (although I question it's usefulness at all).
Related details
- Adam Ren (xWF)
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Ren (Gerrit)
Adam Ren added 6 comments![]( "Open in Gerrit")
Support WebXR MeshingAdam RenLooks like a merge conflict; can you try a rebase?
Done
<<<<<<< HEAD
import org.chromium.chrome.browser.tabmodel.SupportedProfileType;
=======
import org.chromium.chrome.browser.tabmodel.EmptyTabModel;
>>>>>>> f5190562ab669 (Support WebXR Meshing)Adam RenMerge conflict
Done
// 使用 std::span 提供边界检查Adam RenNIT: English for comments
Done
// Copyright 2025 The Chromium AuthorsAdam Ren2026
Done
// Copyright 2019 The Chromium AuthorsAdam Ren2026
Done
// Copyright 2024 The Chromium Authors- Adam Ren (xWF)
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper added 11 comments![]( "Open in Gerrit")
configs -= [ "//build/config/clang:find_bad_constructs" ]It shouldn't be needed here. Openxr_scene_meshing_android.h should only be added in device/vr. Further, you can typically suppress it at only the specific site, which is generally preferable. MOST of the issues I've seen with the API are array integrations which require working with base::span and maybe just a wrapper with UNSAFE_BUFFERS and a comment labeled //SAFETY. If you share the error you're seeing I can help, but this is almost never the right approach.
configs -= [ "//build/config/clang:find_bad_constructs" ]Same comment as for chrome/browser/BUILD.gn
configs -= [ "//build/config/clang:find_bad_constructs" ]Same comment as chrome/browser/BUILD.gn
configs -= [ "//build/config/clang:find_bad_constructs" ]Same comment as the other build files.
#include "device/vr/openxr/android/openxr_scene_meshing_android.h"Likely only need the xr_android.h once you make the change.
// Copyright 2025 The Chromium Authors2026; Please find and replace all other instances of newly added files that do not have 2026.
return std::ranges::find(capabilities, XR_SPATIAL_CAPABILITY_PLANE_TRACKING_EXT) != capabilities.end();std::ranges::contains is preferred.
std::ranges::find(plane_tracking_components,std::ranges::contains
std::ranges::find(plane_tracking_components,std::ranges::contains
Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
EDIT: All of these. Usually you can modify an editor setting to do this automatically.
if (current_frame_meshes.find(mesh_id) == current_frame_meshes.end()) {std::ranges::contains I believe is the preferred replacement; but can also do base::FindIf if you need the value.
Related details
- Adam Ren (xWF)
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Ren (Gerrit)
Adam Ren added 13 comments![]( "Open in Gerrit")
std::set<uint64_t> accumulated_mesh_ids_;And absl::flat_hash_set as well
Done
std::unordered_map<XrUuid, uint64_t, XrUuidHash, XrUuidEqual> uuid_to_id_;Guidance is to use absl::flat_hash_map for better performance.
https://chromium.googlesource.com/chromium/src/+/main/base/containers/README.md
Done
XrSceneMeshingTrackerANDROID mesh_tracker() const { return mesh_tracker_; }Seems odd this needs to be public?
Good catch. Forgot to remove this.
return data64[0] ^ (data64[1] + 0x9e3779b9 + (data64[0] << 6) + (data64[0] >> 2));What is this? Why can the XrUUID not just be hashed as-is? Why do we need sorting? XrUUIDMSFT is able to do this as a simple strong alias to an ID type, though I think maybe we should use the id generator pattern with a map, I'm wondering if some default hashing is fine here.
Thanks, Alex. I've simplified the hash function to use std::hash<std::string_view> instead of the complex mixing. The simplified hash should be sufficient for UUID hashing. And, we're already using the ID generator pattern (GetOrCreateMeshId() maps XrUuid → uint64_t).
std::copy_n(data_span.begin(), 8, reinterpret_cast<uint8_t*>(&data64[0]));I *think* this is a banned construct. Byte_span_from_ref is better: https://source.chromium.org/chromium/chromium/src/+/main:base/containers/span.h;drc=ded9cdb6eb6253321e19b5bfb01d6c8d57a7aca5;l=1600 (or just as_byte_span). I think there's some concerns with this as well though;
Done
LOG(WARNING) << "xrCreateSceneMeshSnapshotANDROID failed with result="LOG (vs DVLOG or DLOG) should be used VERY sparingly due to the fact that it bloats the production code.
Done
// TODO(mesh-detection): Remove this file once XR_ANDROID_scene_meshing
// is officially added to the OpenXR specification and the Chromium
// third_party/openxr submodule is updated.
//Add this as a block to https://source.chromium.org/chromium/chromium/src/+/main:third_party/openxr/dev/xr_android.h instead.
Done
mojom::XRMeshDetectionDataPtr OpenXrMeshManager::GetDetectedMeshesData(
XrTime frame_time) {
return nullptr;
}
std::optional<XrLocation> OpenXrMeshManager::GetXrLocationFromMesh(
MeshId mesh_id,
const gfx::Transform& mesh_id_from_object) const {
return std::nullopt;
}Just make pure virtual (`= 0` in header).
Done
2026; Please find and replace all other instances of newly added files that do not have 2026.
Done
#include "third_party/blink/renderer/platform/heap/collection_support/heap_vector.h"Keep blank newline after includes
Done
MeshId id() const;Not part of the web IDL, so should be `Id`
Done
Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
EDIT: All of these. Usually you can modify an editor setting to do this automatically.
- Adam Ren (xWF)
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Ren (Gerrit)
Adam Ren added 9 comments![]( "Open in Gerrit")
const size_t vertex_count = vertices.size() / 3;Put 3 in a constant.
Done
for (size_t i = 0; i < vertex_count; ++i) {
DOMFloat32Array* vertex_array = DOMFloat32Array::Create(3);
auto span = vertex_array->AsSpan();
span[0] = vertices[i * 3 + 0]; // x
span[1] = vertices[i * 3 + 1]; // y
span[2] = vertices[i * 3 + 2]; // z
result.push_back(NotShared<DOMFloat32Array>(vertex_array));
}```
base::span vertices_span(vertices);
while (vertices_span.size() >= 3) {
DOMFloat32Array* vertex_array = DOMFloat32Array::Create(3);
vertex_array->AsSpan().copy_from(vertices);
vertices = vertices.subspan(3);
}
```
Done
LOG(ERROR) << __func__ << "Adam2: mesh_id=" << idAdam RenSee Log comments
Done
gfx::Point3F camera_position_{0, 0, 0};
gfx::Vector3dF camera_forward_{0, 0, -1};I'm not seeing this as part of the spec.
The camera culling logic here is needed for performance optimization. Through experimentation, I found that filtering meshes based on camera position and view frustum is necessary to ensure the HTML app can render meshes in real-time. I have videos showing this in the google drive folder I posted in the Google Chat Thread.
bool ShouldShowMeshInView(const XRMesh* mesh,Adam RenNot sure that this is part of the spec?
Done
std::optional<gfx::Transform> mesh_transform = mesh->MojoFromObject();mesh_from_object still. We try to keep a consistent naming scheme with all transforms, even if they are short lived.
Done
gfx::Point3F mesh_pos(mesh_from_object_position should be the end result here FWIW
Done
gfx::Point3F mesh_pos(
mesh_transform->rc(0, 3),
mesh_transform->rc(1, 3),
mesh_transform->rc(2, 3)Use gfx::Transform::Decompose instead, it shouldn't matter for our transforms, but it's a bad habit to not use: https://source.chromium.org/chromium/chromium/src/+/main:ui/gfx/geometry/transform.h;drc=967c368f01062a9d008fcc301843899a01167bff;l=516
Done
float dx = mesh_pos.x() - camera_pos.x();
float dy = mesh_pos.y() - camera_pos.y();
float dz = mesh_pos.z() - camera_pos.z();
float distance = std::sqrt(dx*dx + dy*dy + dz*dz);Can Vector3Df from subtracting the two points not be used to grab the length and get distance that way?
Done
Related details
- Adam Ren (xWF)
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper added 3 comments![]( "Open in Gerrit")
OpenXrMeshManager* GetMeshManager() override;Thinking about this, we should probably modify the top-level `GetMeshManager` to allow creating one if the selected SceneUnderstandingManger can't provide meshes and somehow make meshes optional for a SceneUnderstandingManager. This would be in I think OpenXrExtensionHelper, since our goal is to try to deprecate the XR_ANDROID scene understanding manager, since the OpenXR team intends to remove it's support, but we think the meshing extension will remain.
gfx::Point3F camera_position_{0, 0, 0};
gfx::Vector3dF camera_forward_{0, 0, -1};Adam RenI'm not seeing this as part of the spec.
The camera culling logic here is needed for performance optimization. Through experimentation, I found that filtering meshes based on camera position and view frustum is necessary to ensure the HTML app can render meshes in real-time. I have videos showing this in the google drive folder I posted in the Google Chat Thread.
Did you try having the HTML app do the culling? I can imagine some scenarios where the page may want the data but won't render it.
Related details
- Adam Ren (xWF)
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) added 12 comments![]( "Open in Gerrit")
std::string GetSemanticLabelString(XrSceneMeshSemanticLabelANDROID label) {Should be able to re-use: https://source.chromium.org/chromium/chromium/src/+/main:device/vr/public/mojom/vr_service.mojom;drc=0a156043e74e026eb06dbc58a0501c76bc7bc839;l=501
Strings can be resolved in blink.
Done
return std::ranges::find(capabilities, XR_SPATIAL_CAPABILITY_PLANE_TRACKING_EXT) != capabilities.end();std::ranges::contains is preferred.
Done
LOG(ERROR) << "Adam: OpenXrSpatialMeshManager (Plane-based Mesh) Activated!";Adam Ren (xWF)Remove.
Done
std::ranges::find(plane_tracking_components,Adam Ren (xWF)std::ranges::contains
Done
std::ranges::find(plane_tracking_components,Adam Ren (xWF)std::ranges::contains
Done
struct XRMeshData {Security likes full comments describing structs and each member in .mojom files.
Done
string? semantic_label;Adam Ren (xWF)Use existing SemanticLabel type.
Done
vertices_ = MakeGarbageCollected<FrozenArray<NotShared<DOMFloat32Array>>>(
ConvertVerticesToFloat32Arrays(mesh_data.vertices));For the record, we just had a big performance hit from depth for something probably less intense than this; though I don't know that there's a way around it.
It's been removed. Thanks for the suggestion. It does become faster.
gfx::Point3F camera_position_{0, 0, 0};
gfx::Vector3dF camera_forward_{0, 0, -1};Adam RenI'm not seeing this as part of the spec.
Alexander CooperThe camera culling logic here is needed for performance optimization. Through experimentation, I found that filtering meshes based on camera position and view frustum is necessary to ensure the HTML app can render meshes in real-time. I have videos showing this in the google drive folder I posted in the Google Chat Thread.
Did you try having the HTML app do the culling? I can imagine some scenarios where the page may want the data but won't render it.
Yes, It has been implemented in the latest patch,
if (current_frame_meshes.find(mesh_id) == current_frame_meshes.end()) {std::ranges::contains I believe is the preferred replacement; but can also do base::FindIf if you need the value.
Done
gfx::Point3F camera_position(0, 0, 0);
gfx::Vector3dF camera_forward(0, 0, -1);
if (frame_data->render_info && frame_data->render_info->mojo_from_viewer) {
auto mojo_from_viewer = getPoseMatrix(frame_data->render_info->mojo_from_viewer);
if (mojo_from_viewer) {
camera_position = gfx::Point3F(
mojo_from_viewer->rc(0, 3),
mojo_from_viewer->rc(1, 3),
mojo_from_viewer->rc(2, 3)
);
camera_forward = gfx::Vector3dF(
-mojo_from_viewer->rc(0, 2),
-mojo_from_viewer->rc(1, 2),
-mojo_from_viewer->rc(2, 2)
);
camera_forward.GetNormalized(&camera_forward);
}
}Adam Ren (xWF)See comment about just passing in and using mojo_from_viewer for this directly; (although I question it's usefulness at all).
Done
Related details
- Adam Ren
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) added 8 comments![]( "Open in Gerrit")
configs -= [ "//build/config/clang:find_bad_constructs" ]It shouldn't be needed here. Openxr_scene_meshing_android.h should only be added in device/vr. Further, you can typically suppress it at only the specific site, which is generally preferable. MOST of the issues I've seen with the API are array integrations which require working with base::span and maybe just a wrapper with UNSAFE_BUFFERS and a comment labeled //SAFETY. If you share the error you're seeing I can help, but this is almost never the right approach.
Thanks Alex, I saw the "XR_ANDROID_scene_meshing" has been included in /usr/local/google/code/clankium/src/third_party/openxr/dev/xr_android.h
configs -= [ "//build/config/clang:find_bad_constructs" ]Same comment as for chrome/browser/BUILD.gn
Done
configs -= [ "//build/config/clang:find_bad_constructs" ]Adam Ren (xWF)Same comment as chrome/browser/BUILD.gn
Done
configs -= [ "//build/config/clang:find_bad_constructs" ]Adam Ren (xWF)Same comment as the other build files.
Done
IIRC, This section can be omitted.
Done
#include "device/vr/openxr/android/openxr_scene_meshing_android.h"Likely only need the xr_android.h once you make the change.
Done
camera_position_ = camera_position;
camera_forward_ = camera_forward;All this data should be achievable from the viewer pose (mojo_from_viewer)? Which would be the better more accurate way to do this if needed.
Done
Alexander Cooper (Gerrit)
Alexander Cooper added 6 comments![]( "Open in Gerrit")
Still some architectural concerns remaining open from PS#1, so still haven't done a full review pending discussion/adoption of those bits of feedback.
private void requestSceneUnderstandingPermissionIfNeeded() {Previous comment isn't attached anymore, but permissions requests shouldn't be done here. https://source.chromium.org/chromium/chromium/src/+/main:components/permissions/android/java/src/org/chromium/components/permissions/PermissionUtil.java;drc=3808093d5627d10d9bfe6c5000449b381012b5ab;l=30 handles this as part of requesting the immersive-vr or immersive-ar session.
mesh_data_result->all_meshes_ids.push_back(device::MeshId(mesh_id));Please fix this WARNING reported by ClangTidy: check: modernize-use-emplace
use emplace_back instead of push_back (https://cla...
check: modernize-use-emplace
use emplace_back instead of push_back (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-emplace` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel` or `mac-clang-tidy-rel`)
mesh_data_result->all_meshes_ids.push_back(device::MeshId(mesh_id));Please fix this WARNING reported by ClangTidy: check: modernize-use-emplace
use emplace_back instead of push_back (https://cla...
check: modernize-use-emplace
use emplace_back instead of push_back (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-emplace` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel` or `mac-clang-tidy-rel`)
virtual OpenXrMeshManager* GetMeshManager() = 0;Alexander CooperMy understanding from you was that this didn't strictly need to be integrated with the SceneUnderstandingManager framework? The new version *may* need to be part of it, or at least get the spatial discovery snapshot, but until such time as that's ready, I don't think we should do this part of the architecture yet.
That said, I guess it logically makes sense to be part of the infrastructure, so fine to leave as-is in hindsight.
All of that said, and taking my comment from the other item into account: I think we probably should modify the top-level spot that queries/creates the support for a mesh manager to try this if it exists (or try creating it if it exists), but to be able to create it as a standalone as well if need-be.
meshes_data->all_meshes_ids.push_back(device::MeshId(GetMeshId(entity_id)));Please fix this WARNING reported by ClangTidy: check: modernize-use-emplace
use emplace_back instead of push_back (https://cla...
check: modernize-use-emplace
use emplace_back instead of push_back (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-emplace` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel` or `mac-clang-tidy-rel`)
Related details
- Adam Ren
- Adam Ren (xWF)
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper added 1 comment![]( "Open in Gerrit")
struct XrUuidHash {
size_t operator()(const XrUuid& uuid) const {
std::hash<std::string_view> hasher;
std::string_view uuid_view(reinterpret_cast<const char*>(uuid.data), XR_UUID_SIZE);
return hasher(uuid_view);
}
};
struct XrUuidEqual {
bool operator()(const XrUuid& lhs, const XrUuid& rhs) const {
auto lhs_span = base::as_byte_span(lhs.data);
auto rhs_span = base::as_byte_span(rhs.data);
return std::ranges::equal(lhs_span, rhs_span);
}
};Let's make a helper to convert XrUuid to a base::Uuid, which we could also just send across mojom as the id, or if we feel the string comparison is too much, we could just do the base::Uuid to integer ID mapping: https://source.chromium.org/chromium/chromium/src/+/main:base/uuid.h;drc=e0faa4c7861bcb675b1fc0880814cf41b42f68a7;l=21 is the type. It looks like we'd need to turn the byte string into a hex string, but if you prefer I could *try* to send a messsage to the base OWNERS about leveraging an API to speicfy the value again, but not sure how well that'll go over given it looks to have been removed sometime last year.