Integrate Android mesh manager into OpenXR device and scene understanding [chromium/src : main]
Brandon Jones (Gerrit)
Brandon Jones added 2 comments![]( "Open in Gerrit")
Oops! Sorry, I didn't realize that I missed reviewing this patch in the middle of the rest of them.
if (!mesh_manager_) {Why do we have two different paths for getting a mesh manager?
EDIT: Okay, I see that there's a fallback mesh manager here that constructs the meshes from the hit test planes. It would be useful to have a comment here and maybe elsewhere explaining the difference between the two.
Related details
- Adam Ren (xWF)
- Alexander Cooper
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")
Why do we have two different paths for getting a mesh manager?
EDIT: Okay, I see that there's a fallback mesh manager here that constructs the meshes from the hit test planes. It would be useful to have a comment here and maybe elsewhere explaining the difference between the two.
- 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 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. |