Integrate Android mesh manager into OpenXR device and scene understanding [chromium/src : main]
Alexander Cooper (Gerrit)
Alexander Cooper added 11 comments![]( "Open in Gerrit")
OpenXrMeshManager* GetMeshManager() override;I'm not seeing any reason that the MeshManager needs to be included in the SceneUnderstandingManagerAndroid? From my talks with the OpenXR team, the extensions that are supported by this manager are deprecated and so we'd likely remove this in the future. As mentioned before, it may result in some more complex logic, but we shouldn't tie the mesh stuff to this manager.
: mojo_space_(mojo_space), view_space_(view_space) {I don't see why this needs to be a member variable if this *does* need to stick around in this class.
// TODO: Implement MSFT mesh manager support in future CLTODO's should have bugs associated, and TBH, I don't think this even needs a TODO?
// Prefer the scene understanding manager's mesh manager when available.
if (scene_understanding_manager_) {
if (auto* mesh_manager = scene_understanding_manager_->GetMeshManager()) {
return mesh_manager;
}
}
// Fallback: use mesh_manager_.
if (!mesh_manager_) {
XrSpace view_space = GetReferenceSpace(mojom::XRReferenceSpaceType::kViewer);
if (view_space != XR_NULL_HANDLE) {
mesh_manager_ = extension_helper_->CreateMeshManager(
session_, local_space_, view_space);
}Ideally we just assign this item as part of the setup steps rather than having this logic here, TBH this is the logic that should probably be hidden in the ExtensionHelper::CreateMeshManager class. Note that creating it here is essentially lazy creation which we don't want anymore, as it could mean that we'd not create a mesh_manager_ if it was required.
// requested, use CreateMeshManager directly to avoid CHECK failure in
// CreateSceneUnderstandingManager.We should elaborate on what that is/why it's there not just that there's a CHECK failure.
XrSpace view_space = GetReferenceSpace(mojom::XRReferenceSpaceType::kViewer);Why does the mesh manager need view space? That could be passed in on a per-frame basis, as it's often not something we've created by this point.
OpenXrMeshManager* GetMeshManager() override;ISTM that this is getting tied to the OpenXRSceneUnderstandingManager from the simplification that the SpatialFrameworkManager *is* a SceneUnderstandingManager, and that is perhaps an assumption we should work to break. I guess I'm not sure how we'd do meshes for the MSFT extension if that needs to be incorporated in it's framework, but I don't think the other approach you've created for meshes *requires* that it be part of the Android SceneUnderstandingManager?
The original purpose of the SceneUnderstandingManager was to ensure that the "managers" or "features" that needed to be guaranteed that the same underlying system was being used could be guaranteed to be used (e.g. Planes/Hit-Tests/Anchors all interact with each other at a low-level). I don't think that's the state for the SceneUnderstandingManager *in general*, though the spatial entities one obviously still uses the same framework and falls-back to using plane data?
A full split of how spatial entites stuff is handled is an unfair split to ask of you, so probably worth a bug to be filed; but we shouldn't e.g. shoe-horn the other feature into the OpenXrSceneUnderstandingManagerANDROID (which is really meant to be the XR_ANDROID trackables framework), just because of it. We should leverage some separate fallback logic for it's creation based on e.g. that knowledge.
// TODO: MESH_DETECTION should have its own ::IsSupported check
// (e.g. OpenXrMeshManager::IsSupported()).Please do this before landing
std::optional<device::Pose> TryGetMojoFromMesh(uint64_t mesh_id) const;MeshId, never use the bare uint64_t for IDs. (And below)
const char* AlignmentToString(XrSpatialPlaneAlignmentEXT alignment) {
switch (alignment) {
case XR_SPATIAL_PLANE_ALIGNMENT_HORIZONTAL_UPWARD_EXT:
return "HORIZONTAL_UP";
case XR_SPATIAL_PLANE_ALIGNMENT_HORIZONTAL_DOWNWARD_EXT:
return "HORIZONTAL_DOWN";
case XR_SPATIAL_PLANE_ALIGNMENT_VERTICAL_EXT:
return "VERTICAL";
case XR_SPATIAL_PLANE_ALIGNMENT_ARBITRARY_EXT:
return "ARBITRARY";
default:
return "UNKNOWN";
}
}Just print the number or convert it to the mojo type and print that number. In general this just increases the binary size for debug data.
// TODO: Consider merging OpenXrSpatialMeshManager into OpenXrSpatialPlaneManager
// since mesh detection here is tightly coupled with plane tracking (Mesh2D is a
// component of XR_SPATIAL_CAPABILITY_PLANE_TRACKING_EXT). If a more general
// mesh manager is needed later, it can be created separately and receive a
// reference to the plane manager.I think we should just do this now, as there's a *lot* of duplicated logic with the plane tracking code here.
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 11 comments![]( "Open in Gerrit")
I'm not seeing any reason that the MeshManager needs to be included in the SceneUnderstandingManagerAndroid? From my talks with the OpenXR team, the extensions that are supported by this manager are deprecated and so we'd likely remove this in the future. As mentioned before, it may result in some more complex logic, but we shouldn't tie the mesh stuff to this manager.
Sure, it has been removed from android manager.
: mojo_space_(mojo_space), view_space_(view_space) {I don't see why this needs to be a member variable if this *does* need to stick around in this class.
It was kept for optimization purpose so that the queried submeshes can be centered around the user's view_space. I have switched to use "passing in on a per-frame basis"
// TODO: Implement MSFT mesh manager support in future CLTODO's should have bugs associated, and TBH, I don't think this even needs a TODO?
Yes, it has been removed.
// Prefer the scene understanding manager's mesh manager when available.
if (scene_understanding_manager_) {
if (auto* mesh_manager = scene_understanding_manager_->GetMeshManager()) {
return mesh_manager;
}
}
// Fallback: use mesh_manager_.
if (!mesh_manager_) {
XrSpace view_space = GetReferenceSpace(mojom::XRReferenceSpaceType::kViewer);
if (view_space != XR_NULL_HANDLE) {
mesh_manager_ = extension_helper_->CreateMeshManager(
session_, local_space_, view_space);
}Ideally we just assign this item as part of the setup steps rather than having this logic here, TBH this is the logic that should probably be hidden in the ExtensionHelper::CreateMeshManager class. Note that creating it here is essentially lazy creation which we don't want anymore, as it could mean that we'd not create a mesh_manager_ if it was required.
Done
// requested, use CreateMeshManager directly to avoid CHECK failure in
// CreateSceneUnderstandingManager.We should elaborate on what that is/why it's there not just that there's a CHECK failure.
No scene understanding manager involvement, no comment about CHECK failures. This review comment is already resolved by the changes we made.
XrSpace view_space = GetReferenceSpace(mojom::XRReferenceSpaceType::kViewer);Why does the mesh manager need view space? That could be passed in on a per-frame basis, as it's often not something we've created by this point.
Done
ISTM that this is getting tied to the OpenXRSceneUnderstandingManager from the simplification that the SpatialFrameworkManager *is* a SceneUnderstandingManager, and that is perhaps an assumption we should work to break. I guess I'm not sure how we'd do meshes for the MSFT extension if that needs to be incorporated in it's framework, but I don't think the other approach you've created for meshes *requires* that it be part of the Android SceneUnderstandingManager?
The original purpose of the SceneUnderstandingManager was to ensure that the "managers" or "features" that needed to be guaranteed that the same underlying system was being used could be guaranteed to be used (e.g. Planes/Hit-Tests/Anchors all interact with each other at a low-level). I don't think that's the state for the SceneUnderstandingManager *in general*, though the spatial entities one obviously still uses the same framework and falls-back to using plane data?
A full split of how spatial entites stuff is handled is an unfair split to ask of you, so probably worth a bug to be filed; but we shouldn't e.g. shoe-horn the other feature into the OpenXrSceneUnderstandingManagerANDROID (which is really meant to be the XR_ANDROID trackables framework), just because of it. We should leverage some separate fallback logic for it's creation based on e.g. that knowledge.
Acknowledged. In this CL, mesh has been fully decoupled from OpenXRSceneUnderstandingManagerAndroid — it's now created and owned standalone by OpenXrApiWrapper. Filed bug b/492887842 to track splitting mesh out of the spatial framework in a future CL.
// TODO: MESH_DETECTION should have its own ::IsSupported check
// (e.g. OpenXrMeshManager::IsSupported()).Please do this before landing
Done
std::optional<device::Pose> TryGetMojoFromMesh(uint64_t mesh_id) const;MeshId, never use the bare uint64_t for IDs. (And below)
Done
const char* AlignmentToString(XrSpatialPlaneAlignmentEXT alignment) {
switch (alignment) {
case XR_SPATIAL_PLANE_ALIGNMENT_HORIZONTAL_UPWARD_EXT:
return "HORIZONTAL_UP";
case XR_SPATIAL_PLANE_ALIGNMENT_HORIZONTAL_DOWNWARD_EXT:
return "HORIZONTAL_DOWN";
case XR_SPATIAL_PLANE_ALIGNMENT_VERTICAL_EXT:
return "VERTICAL";
case XR_SPATIAL_PLANE_ALIGNMENT_ARBITRARY_EXT:
return "ARBITRARY";
default:
return "UNKNOWN";
}
}Just print the number or convert it to the mojo type and print that number. In general this just increases the binary size for debug data.
Done
// TODO: Consider merging OpenXrSpatialMeshManager into OpenXrSpatialPlaneManager
// since mesh detection here is tightly coupled with plane tracking (Mesh2D is a
// component of XR_SPATIAL_CAPABILITY_PLANE_TRACKING_EXT). If a more general
// mesh manager is needed later, it can be created separately and receive a
// reference to the plane manager.I think we should just do this now, as there's a *lot* of duplicated logic with the plane tracking code here.
The spatial entity mesh API is still not fully ready. I'd prefer to defer this merge. If the mesh2D component ends up working differently than expected, we'd have to undo the merge.
Related details
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
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")
// TODO: Consider merging OpenXrSpatialMeshManager into OpenXrSpatialPlaneManager
// since mesh detection here is tightly coupled with plane tracking (Mesh2D is a
// component of XR_SPATIAL_CAPABILITY_PLANE_TRACKING_EXT). If a more general
// mesh manager is needed later, it can be created separately and receive a
// reference to the plane manager.Adam Ren (xWF)I think we should just do this now, as there's a *lot* of duplicated logic with the plane tracking code here.
The spatial entity mesh API is still not fully ready. I'd prefer to defer this merge. If the mesh2D component ends up working differently than expected, we'd have to undo the merge.
Right but then we essentially just have a handful of methods and not a whole class?
Further, with the switch in the constructor logic, I think this potentially has no way to get created anymore? (And even if it did, I think we'd be 'double creating' the spatial entities framework manager, which is potentially problematic).
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 1 comment![]( "Open in Gerrit")
// TODO: Consider merging OpenXrSpatialMeshManager into OpenXrSpatialPlaneManager
// since mesh detection here is tightly coupled with plane tracking (Mesh2D is a
// component of XR_SPATIAL_CAPABILITY_PLANE_TRACKING_EXT). If a more general
// mesh manager is needed later, it can be created separately and receive a
// reference to the plane manager.Adam Ren (xWF)I think we should just do this now, as there's a *lot* of duplicated logic with the plane tracking code here.
Alexander CooperThe spatial entity mesh API is still not fully ready. I'd prefer to defer this merge. If the mesh2D component ends up working differently than expected, we'd have to undo the merge.
Right but then we essentially just have a handful of methods and not a whole class?
Further, with the switch in the constructor logic, I think this potentially has no way to get created anymore? (And even if it did, I think we'd be 'double creating' the spatial entities framework manager, which is potentially problematic).
Sure, I have merged the mesh manager into the plane manager.
Related details
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
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")
Overall seems fine to me; however as I'm going OOO, I'll defer to Brandon.
if (mesh_detection_enabled_) {Would you not want to use the polygon if it's present?
return std::nullopt;Add TODO or comment
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) voted and added 2 comments![]( "Open in Gerrit")
Votes added by Adam Ren (xWF)
| Commit-Queue | +1 |
2 comments
Would you not want to use the polygon if it's present?
Thanks Alex, it was a simpler MVP initial implementation. I have implemented the polygon-based meshing solution.
return std::nullopt;Adam Ren (xWF)Add TODO or comment
Done
Related details
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Sheedy (Gerrit)
Brian Sheedy added 1 comment![]( "Open in Gerrit")
// TODO: Implement mesh-based native origin lookup.Please use `TODO(crbug.com/#####):` referring to the relevant bug instead of a generic TODO.
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. |
Adam Ren (xWF) (Gerrit)
Adam Ren (xWF) added 1 comment![]( "Open in Gerrit")
Please use `TODO(crbug.com/#####):` referring to the relevant bug instead of a generic TODO.
- Alexander Cooper
- Brandon Jones
- Brian Sheedy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Sheedy (Gerrit)
Brian Sheedy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Sheedy
| Code-Review | +1 |
1 comment
Looks fine at a glance, but I have basically no knowledge about this feature. Deferring to Brandon for a more proper review, but stamping since alcooper@ is OOO for a while.
Related details
- Adam Ren (xWF)
- Alexander Cooper
- Brandon Jones
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |