Skip to content

Fix distant pickup - #891

Merged
BastiaanOlij merged 1 commit into
GodotVR:masterfrom
renaiku:master
Oct 8, 2026
Merged

BastiaanOlij merged 1 commit into
GodotVR:masterfrom
renaiku:master

Conversation

@renaiku

@renaiku renaiku commented Sep 17, 2026 •

Copy link
Copy Markdown

The PR #804 introduced a bug in the function pickup _on_ranged_entered() by rejecting every Node without a can_ranged_grab function.

I think most of the scenes, like the basketball, relies on a var can_ranged_grab, and they are rejected.

Can you confirm @mubinulhaque ?
(btw thank you for all these typings everywhere 😍)

@mubinulhaque

Copy link
Copy Markdown
Contributor

As far as I can see, the code is the same, and the part that says or not target.can_ranged_grab should be checking for the variable.

Nevertheless, I checked out your branch, and couldn't find any difference. Which demo are you testing?

@renaiku

renaiku commented Sep 17, 2026

Copy link
Copy Markdown
Author

I've updated my addon version in my project and this change broke the distant grab.

Indeed when reading again my commit, it should be better, I will rework it, because it just reversed the problem but with the variable instead.

The new code with typings introduced that if the pickable doesn't have the function, it will return (even if it has the variable)

if not target.has_method('can_ranged_grab') or not target.can_ranged_grab:
    return

@mubinulhaque

Copy link
Copy Markdown
Contributor

I see what it is now. Because of the line if not target.has_method('can_ranged_grab') or not target.can_ranged_grab:, it immediately returns once the method is not found.

This should just be changed to if not "can_ranged_grab" in target or target.can_ranged_grab:. We should not cast target to XRToolsPickable, because then that would exclude XRToolsSnapZones.

@renaiku

renaiku commented Sep 17, 2026

Copy link
Copy Markdown
Author

Oh nice catch for the snap zone. Should I update the PR with your code proposal or I met you handle this and I close this PR ?

@mubinulhaque

Copy link
Copy Markdown
Contributor

Updating the PR you have here is fine.

@mubinulhaque

mubinulhaque commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

There is a problem with the new changes: we are now rejecting ranged grabs for objects that explicitly enable them. Instead of:
if not "can_ranged_grab" in target or target.can_ranged_grab:
we should put:
if not "can_ranged_grab" in target or not target.can_ranged_grab:

@renaiku

renaiku commented Sep 18, 2026

Copy link
Copy Markdown
Author

damned I copy pasted what you wrote without reading 🥲

@mubinulhaque

Copy link
Copy Markdown
Contributor

Can you squash your commits?

@renaiku

renaiku commented Oct 8, 2026

Copy link
Copy Markdown
Author

Sorry for the delay

@BastiaanOlij BastiaanOlij added the bug Something isn't working label Oct 8, 2026
@BastiaanOlij BastiaanOlij added this to the 4.6.0 milestone Oct 8, 2026

@BastiaanOlij BastiaanOlij left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@BastiaanOlij
BastiaanOlij merged commit d15a51e into GodotVR:master Oct 8, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants