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

Use the move bar to freely position windows #1670

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Use the move bar to freely position windows #1670

wants to merge 8 commits into from

Conversation

svillar
Copy link
Member

@svillar svillar commented Jan 7, 2025

Users should be able to move the windows around (although the 3 windows will still be grouped together) by clicking and dragging in the recently added move bar bellow the window.

Window distance to the user would require additional changes and it is not included in this PR

Fixes #99, fixes #927

@svillar svillar mentioned this pull request Jan 7, 2025
Closed
@svillar
Copy link
Member Author

svillar commented Jan 7, 2025

Demo of the new behaviour

movingwindows28.mp4

Compare it to what was happening previously

ScreenRecording_2024.12.02-12.17.59.mp4

@felipeerias
Copy link
Collaborator

@svillar This is a great improvement!

It is a pity that the window moves downward when clicking the first time. Maybe we need to apply a transform taking into account the hit point where the click has happened in the window widget. I've had a look at the code and I think that we can gather all the information that we need, but it seems a bit complex to implement.

It would be awesome to implement the grab button as well, but we can leave that for another PR.

@svillar
Copy link
Member Author

svillar commented Jan 8, 2025

@svillar This is a great improvement!

It is a pity that the window moves downward when clicking the first time. Maybe we need to apply a transform taking into account the hit point where the click has happened in the window widget. I've had a look at the code and I think that we can gather all the information that we need, but it seems a bit complex to implement.

Yeah, I agree is the main drawback. Should we land this as is, or do you prefer to fix this issue first.

It would be awesome to implement the grab button as well, but we can leave that for another PR.

Yeah, I think we can treat it as an enhancement

@svillar svillar added the release_candidate PR that should be part of the next release label Jan 14, 2025
@javifernandez
Copy link
Member

Yeah, I agree is the main drawback. Should we land this as is, or do you prefer to fix this issue first.

It's quite annoying, tbh. I have mixed feelings, because the feature is nice and something users have been requested since long ago. How difficult would it be to fix ?

felipeerias
felipeerias approved these changes Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@felipeerias felipeerias left a comment

Choose a reason for hiding this comment

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

LGTM. The feature works well enough, I think. We can leave the improvements (window "jump", etc.) for future PRs.

@svillar
Copy link
Member Author

svillar commented Jan 15, 2025

Yeah, I agree is the main drawback. Should we land this as is, or do you prefer to fix this issue first.

It's quite annoying, tbh. I have mixed feelings, because the feature is nice and something users have been requested since long ago. How difficult would it be to fix ?

Not trivial at least. I can give it a try though

@javifernandez
Copy link
Member

BTW, this change causes a regression in the headklock functionality. It's not possible to lock the window based on the headset orientation.

@svillar
Copy link
Member Author

svillar commented Jan 15, 2025

BTW, this change causes a regression in the headklock functionality. It's not possible to lock the window based on the headset orientation.

It works fine here. See the video:

headlock.mp4

javifernandez
javifernandez requested changes Jan 15, 2025
View reviewed changes
app/src/main/cpp/DeviceDelegate.h Outdated Show resolved Hide resolved
app/src/openxr/cpp/DeviceDelegateOpenXR.cpp Outdated Show resolved Hide resolved
app/src/wavevr/cpp/DeviceDelegateWaveVR.cpp Outdated Show resolved Hide resolved
javifernandez
javifernandez requested changes Jan 15, 2025
View reviewed changes
Copy link
Member

@javifernandez javifernandez left a comment

Choose a reason for hiding this comment

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

The fix for the headlock regression worked, thanks.

I have some comments about the code, see below inline.

javifernandez
javifernandez requested changes Jan 16, 2025
View reviewed changes
Copy link
Member

@javifernandez javifernandez left a comment

Choose a reason for hiding this comment

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

The code looks good now.

In terms of functionality, this new version works much better now. Awesome ! The z-axis movement is fluid and clear now, and it's more stable as it's not affected by unconscious movements on the other axes.

On the bad news, I've found out a bug, which I don't think it's a regression but something this PR has not considered. When enabling the headlock, it's still possible to touch the "free movement" bar and start moving the window. The headlock is stopped, but the radio-button in the settings dialog is still enabled.

@svillar
Use the move bar widget to freely position the windows
3dbc69a 
Users should be able to move the windows around (although the 3 windows will still be
grouped together) by clicking and dragging in the recently added move bar bellow
the browser window.

This required a small refactoring in head lock mode. From now on, instead of having
a head lock on/off switch, we have a head lock mode, which can take three different
values: no lock (normal mode), head lock and controller lock (the one used to move
windows).

Fixes #99, fixes #927
@svillar
Enable smooth window movement
8c74221 
Currently when moving windows, they closely follow the orientation of
the controller. That means that they are prone to the natural small
shaking of human hands. To alleviate that, we can add some linear
interpolation between consecutive orientations. By using a value close
to 0 (in this case 0.1) we ensure that the interpolated value is very
close to the previous value, and thus, the movement is very smooth.
@svillar
Change window distance with the moving bar
47a5c00 
Users can now modify the distante to the browser window when dragging
the moving bar widget by moving the controller/hand closer or further
away.

As this can be done with a gesture now there is no need to have a
dedicated widget in the settings for that.
@svillar
Review comments
80aaa12
@svillar svillar requested a review from javifernandez January 16, 2025 15:40
@svillar
Copy link
Member Author

svillar commented Jan 16, 2025

The code looks good now.

In terms of functionality, this new version works much better now. Awesome ! The z-axis movement is fluid and clear now, and it's more stable as it's not affected by unconscious movements on the other axes.

On the bad news, I've found out a bug, which I don't think it's a regression but something this PR has not considered. When enabling the headlock, it's still possible to touch the "free movement" bar and start moving the window. The headlock is stopped, but the radio-button in the settings dialog is still enabled.

Good catch. Fixed now

javifernandez
javifernandez approved these changes Jan 16, 2025
View reviewed changes
Copy link
Member

@javifernandez javifernandez left a comment

Choose a reason for hiding this comment

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

It's perfect now. Thanks !

@svillar svillar force-pushed the moving_windows branch 2 times, most recently from d4eae53 to cc3b4cf Compare January 17, 2025 11:04
@javifernandez javifernandez self-requested a review January 17, 2025 12:56
javifernandez
javifernandez approved these changes Jan 17, 2025
View reviewed changes
Copy link
Member

@javifernandez javifernandez left a comment

Choose a reason for hiding this comment

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

I tested the change to avoid the big initial repositioning and it's not better. It places the window slightly upper the current position, which is better than downwards. Ideally, I'd try to reduce the distance, though.

Anyway, since this is a temporary solution, I think we can live until we solved it properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javifernandez javifernandez javifernandez approved these changes

@felipeerias felipeerias felipeerias approved these changes

Assignees
No one assigned
Labels
release_candidate PR that should be part of the next release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drag and rotate windows using the controller Click and Drag windows
3 participants
@svillar @felipeerias @javifernandez