Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed the second controller doesn't show up problem #989

Merged
merged 2 commits into from
Mar 9, 2019
Merged

Fixed the second controller doesn't show up problem #989

merged 2 commits into from
Mar 9, 2019

Conversation

daoshengmu
Copy link
Contributor

Per #977. We find the second controller does not show up intermittently, and I notice it is because the model vector of ControllerContainer.cpp is not resized properly.

@daoshengmu daoshengmu self-assigned this Mar 8, 2019
@ghost ghost added the in progress label Mar 8, 2019
bluemarvin
bluemarvin approved these changes Mar 8, 2019
View reviewed changes
Copy link
Contributor

@bluemarvin bluemarvin left a comment

Choose a reason for hiding this comment

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

I think this code would benefit from some clean up. I wonder if we could make it more flexible by keying off the reported caps instead of the type? And only use the type for the controller model file mapping?

app/src/main/cpp/ControllerContainer.cpp
@@ -44,7 +44,7 @@ struct ControllerContainer::State {
}

void SetUpModelsGroup(const int32_t aModelIndex) {
if (models.size() >= aModelIndex) {
if (aModelIndex >= models.size()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch.

app/src/oculusvr/cpp/DeviceDelegateOculusVR.cpp
// the current workaround is checking if it is Quest.
return IsOculusQuest();
}

Copy link
Contributor

@bluemarvin bluemarvin Mar 8, 2019 •
edited by philip-lamb

Choose a reason for hiding this comment

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

I find having two different Is6DOF() functions confusing. Also is it possible to use a Go 3DoF controller with 6DoF headsets?

Copy link
Contributor Author

@daoshengmu daoshengmu Mar 8, 2019 •
edited

Choose a reason for hiding this comment

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

One of Is6DOF() is for controllers, and the other one is for headsets. Currently, we only use ControllerState.Is6DOF() for getting the capability from controllers. We didn't have Is6DOF() requirement for headsets yet, I just prepare it for the future. Ideally, ovrInputHeadsetCapabilities should tell us the capability flag of headsets, but it doesn't work right now, and I have checked OVR mobile samples are also under the same situation.

I have done a try for using Go 3DOF controller with the Oculus 6DOF headset, I didn't see any possibility.

@daoshengmu
Copy link
Contributor Author

I think this code would benefit from some clean up. I wonder if we could make it more flexible by keying off the reported caps instead of the type? And only use the type for the controller model file mapping?

yep. That's what I was doing. Probably, I am not doing good enough :)

@daoshengmu daoshengmu merged commit 38c334e into MozillaReality:master Mar 9, 2019
@ghost ghost removed the in progress label Mar 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluemarvin bluemarvin bluemarvin approved these changes

@MortimerGoro MortimerGoro MortimerGoro approved these changes

Assignees

@daoshengmu daoshengmu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@daoshengmu @bluemarvin @MortimerGoro