Skip to content
This repository has been archived by the owner on Aug 11, 2024. It is now read-only.

Only apply default head height if not managed by device #657

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

FejZa
Copy link
Contributor

@FejZa FejZa commented Jul 30, 2020

XRTK - Mixed Reality Toolkit Pull Request

Overview

Introduced

/// <summary>
/// Is the head height, and thus the camera y-position, managed by the device itself?
/// If true, the <see cref="DefaultHeadHeight"/> setting is ignored and has no effect
/// on camera positioning.
/// </summary>
bool HeadHeightIsManagedByDevice { get; }

to IMixedRealityCameraDataProvider and implemented as

/// <inheritdoc />
public virtual bool HeadHeightIsManagedByDevice => XRDevice.isPresent;

in BaseCameraDataProvider

to only apply the DefaultHeadHeight setting, if the device does not manage head height itself, which is the case
with many devices, like HoloLens e.g.

By default we will assume as soon as an XR Device is connected that it manages head height, since most devices these days will do. The developer has the option to override the default value per platform.

Changes

@FejZa FejZa added the In Progress PR currently still being developed label Jul 30, 2020
@FejZa FejZa self-assigned this Jul 30, 2020
@FejZa FejZa added this to the Version 1.0.0 milestone Jul 30, 2020
@FejZa FejZa added Ready for review PR finished primary development, open for review and removed In Progress PR currently still being developed labels Aug 3, 2020
@FejZa FejZa requested a review from SimonDarksideJ August 3, 2020 20:14
@FejZa
Copy link
Contributor Author

FejZa commented Aug 3, 2020

@SimonDarksideJ I tested this and it did resolve the issues I had. Ready for review.

Copy link
Contributor

@StephenHodgson StephenHodgson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with these changes but I'm not sure if we need to update and validate the other platforms that bypass the XR Subsystems that need to also check if the device is present.

@FejZa
Copy link
Contributor Author

FejZa commented Aug 5, 2020

True, I'll double check the platform overrides.

Copy link
Contributor

@SimonDarksideJ SimonDarksideJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, tested on WMR, Oculus and OpenVR. Hands now appear correctly on all these platforms.

@SimonDarksideJ SimonDarksideJ merged commit a27b501 into development Aug 25, 2020
@SimonDarksideJ SimonDarksideJ deleted the fix/656-head-height branch August 25, 2020 11:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Ready for review PR finished primary development, open for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DefaultHeadHeight setting in camera data provider profile causes misbehaviour
3 participants