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.

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

Add SnapPath #685

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Add SnapPath #685

wants to merge 18 commits into from

Conversation

squidt
Copy link

@squidt squidt commented Oct 3, 2024

Extends from SnapZone and allows the user to specify a Path3D to which PickableObjects will be snapped. Snapping is done by creating a temporary SnapZone in-script along the closest point of the Path3D node. This temporary SnapZone is freed once the PickableObject is taken from the rail. Make snapping discrete with the "snap_interval", the distance between snaps in Meters.

image

Only set radius if the collision shape has that property, to allow things like boxes or polygons for SnapPath.
@DigitalN8m4r3
Copy link
Contributor

hmm, this looks really interesting...
most def gotta check this out, i wonder at the same time if this could be adapted for more usecases such
as slideable grabpoints which go along the line of the path and slideable magazines for firearms etc.
aynways, needs testing and as i can see there is no actual demo that comes with it?
you may want to add that so that the idea and functionality can be tested

@BastiaanOlij BastiaanOlij added the enhancement New feature or request label Oct 4, 2024
@DigitalN8m4r3
Copy link
Contributor

Extends from SnapZone and allows the user to specify a Path3D to which PickableObjects will be snapped. Snapping is done by creating a temporary SnapZone in-script along the closest point of the Path3D node. This temporary SnapZone is freed once the PickableObject is taken from the rail. Make snapping discrete with the "snap_interval", the distance between snaps in Meters.

sorry but i just fail to get this working, could it be that something is missing or am i just not figuring this out the way its supposed to be...

@squidt
Copy link
Author

squidt commented Oct 7, 2024

sorry but i just fail to get this working, could it be that something is missing or am i just not figuring this out the way its supposed to be...

I'll work on adding a demo right away

@DigitalN8m4r3
Copy link
Contributor

just tested the updated version, lookin good for me
@Malcolmnixon you might want to give it a spin, it is a nice addition
@squidt not sure if you are interested but maybe you can follow up on my initial message regarding the expansion of this, anyways good work there!

@squidt
Copy link
Author

squidt commented Oct 7, 2024

I accidentally made all these commits using 4.3.stable and I just noticed the repo is using 4.2 in the .project so they might not work for that version. "Parse Error: Saved with newer project format version". I'll try to fix that.
@DigitalN8m4r3 I have been interested in making an InteractablePath for objects like sniper bolts. I might work on that or something like a SnapArea that Pickables can 'float' in like many VR games have for backpacks.

@squidt
Copy link
Author

squidt commented Oct 16, 2024

XRToolsPathGuide uses a Marker3D to define a length for XRToolsSnapPath to use so it can place the object fully on the path with no overhanging. Example added to the PicatinnyScope. The next feature I plan on adding would be using a pickable's existing collision shape as the shape used to pick it up off the rail instead of a circle that might be offset or wrongly sized. I believe there is a way to do that in-code, I'll try it later.

@DigitalN8m4r3
Copy link
Contributor

XRToolsPathGuide uses a Marker3D to define a length for XRToolsSnapPath to use so it can place the object fully on the path with no overhanging. Example added to the PicatinnyScope. The next feature I plan on adding would be using a pickable's existing collision shape as the shape used to pick it up off the rail instead of a circle that might be offset or wrongly sized. I believe there is a way to do that in-code, I'll try it later.

any Chance of expqndong this to a optional enforcing to use the furthest point along a predefined axis/ selectable from x,y,z and if not reaching the closest point(closest to the pickable) = drop instead of snap. This could be useful for snapable mags.

@squidt
Copy link
Author

squidt commented Oct 16, 2024

any Chance of expqndong this to a optional enforcing to use the furthest point along a predefined axis/ selectable from x,y,z and if not reaching the closest point(closest to the pickable) = drop instead of snap. This could be useful for snapable mags.

Sorry I don't understand the question. I would like to make a solution for all the different types of magazines but that would be more appropriate to the 'interactable' classes where the user is driving a motion, then upon reaching the end the magazine is inserted. Examples such as an AK magazine where you have to rock it in, or a bolt handle you have to pull up to unlock before pulling back. I'll try to come up with a solution for that but it'll likely end up in a different pull request using a new class, where the final example may combine classes in the node tree to get a desired effect.

@DigitalN8m4r3
Copy link
Contributor

any Chance of expqndong this to a optional enforcing to use the furthest point along a predefined axis/ selectable from x,y,z and if not reaching the closest point(closest to the pickable) = drop instead of snap. This could be useful for snapable mags.

Sorry I don't understand the question. I would like to make a solution for all the different types of magazines but that would be more appropriate to the 'interactable' classes where the user is driving a motion, then upon reaching the end the magazine is inserted. Examples such as an AK magazine where you have to rock it in, or a bolt handle you have to pull up to unlock before pulling back. I'll try to come up with a solution for that but it'll likely end up in a different pull request using a new class, where the final example may combine classes in the node tree to get a desired effect.

Aah this is true, sorry that i bothered you but indeed your theoretical approach would be way better.
Anyways thanks for sharing your work and am lookin forward to testing it 😀

@squidt
Copy link
Author

squidt commented Oct 16, 2024

Aah this is true, sorry that i bothered you but indeed your theoretical approach would be way better. Anyways thanks for sharing your work and am lookin forward to testing it 😀

No worries you didn't bother me at all! It's gonna be fun seeing what can be done.

Copy link
Contributor

@DigitalN8m4r3 DigitalN8m4r3 left a comment

Choose a reason for hiding this comment

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

lgtm, tested in and out and it is not interfeering with anything.
even if my approval doesnt count when it comes to merging, i can at least say that i tested it and there are no issues!

Copy link
Member

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

Finally had time to have a closer look at the code side of this. Looks very clean!
Malcolm needs to have a quick pass over this but not sure if he'll have time quickly, but imho this can be merged as soon as he gives his ok.

addons/godot-xr-tools/objects/snap_path.gd Outdated Show resolved Hide resolved
@BastiaanOlij BastiaanOlij added this to the 4.4.0 milestone Oct 21, 2024
Previously adding a snap_interval would move objects one snap_interval forward and prevent objects from snapping backwards over the edge of the path. This solution checks and solves for this edge case. If the object were to snap its front (p1) further back than the max-backwards position (path_p2 + _length) it will round up to the nearest snap_interval, moving it forwards.
- Add clarifying comments
more_snap should have been given p1_new instead of ideal_snap. Previously both values would be the same. Now works as expected, the PicatinnyScope in the demo scene will snap forward on the rail instead of overhanging when placed at the very back of the rail.
When a pickable object is placed on a snap_path the pickable's CollisionShape3Ds are used to detect grabbing from the rail. This behavior is more intuitive for the class user and prevents desync between a temp snap_zone's automatic circle shape and the pickable. It will be more computationally expensive depending on the pickable's shapes.

- Remove errors from missing expected children by using has_node() before get_node()
@squidt
Copy link
Author

squidt commented Oct 22, 2024

I just found out about gdlint and installed it. I'll make a commit to fix whatever it brings up.

- Shorten descriptions
- Reorder 'extends'
- Remove whitespace
- Remove unused function
- Remove unnecessary else: return
- Fix 'start_xr.gd' static variable declared out of order
     - Unsure if this fix should be in this PR, but it clears the last of the gdlint errors even though this file was not previously changed in this PR.
@squidt
Copy link
Author

squidt commented Oct 25, 2024

any Chance of expqndong this to a optional enforcing to use the furthest point along a predefined axis/ selectable from x,y,z and if not reaching the closest point(closest to the pickable) = drop instead of snap. This could be useful for snapable mags.

I think I know what you mean by this now. Something like: A magazine path where the user brings a magazine close and it attaches to the path while close enough and still grabbed. The user then has to insert it fully to snap it or else 'when dropped' it will slide out/ fall. It sounds possible with the current classes I might work on making an example.

I'm experimenting more with the 'interactable' classes. I was able to make a charging handle that can be slid back until the end then tilted up to be locked. I could make a PR request implementing that for the sniper rifle but I did it with a custom slider class and a couple changes to the source of those classes.

It'll just depend on what I work on next, I've been prototyping a game with this plugin and I'll be trying to bring back any improvements or examples I find useful.

@DigitalN8m4r3
Copy link
Contributor

any Chance of expqndong this to a optional enforcing to use the furthest point along a predefined axis/ selectable from x,y,z and if not reaching the closest point(closest to the pickable) = drop instead of snap. This could be useful for snapable mags.

I think I know what you mean by this now. Something like: A magazine path where the user brings a magazine close and it attaches to the path while close enough and still grabbed. The user then has to insert it fully to snap it or else 'when dropped' it will slide out/ fall. It sounds possible with the current classes I might work on making an example.

I'm experimenting more with the 'interactable' classes. I was able to make a charging handle that can be slid back until the end then tilted up to be locked. I could make a PR request implementing that for the sniper rifle but I did it with a custom slider class and a couple changes to the source of those classes.

It'll just depend on what I work on next, I've been prototyping a game with this plugin and I'll be trying to bring back any improvements or examples I find useful.

Am sure this would be welcome, especialy since we are approaching the end of xr tools 1, v2 most probably wont include much thirdparty game specific stuff (i might be wrong but thats what i imagined from reading about it)

So yes, please go ahead if your solution works better then mine, am happy with mine getting ditched in favor of something more robust

@squidt
Copy link
Author

squidt commented Nov 2, 2024

So yes, please go ahead if your solution works better then mine, am happy with mine getting ditched in favor of something more robust

Thanks! it will be a breaking change but hopefully it ends up working better. The idea behind the custom slider is that the user can rotate the slider node with the Editor's rotation gizmos and using a 'private Transform3D' we save an origin point and rotation whenever there is a transform change from non-interactable-driven-behavior. Then the slider always slides 'back' from however its been rotated. So far I have it working for the slider and hinge.

The benefits are that there's no more SliderOrigin HingeOrigin nodes required, and the setup feels more intuitive from a user perspective, rotating the slider node into place rather than changing the surrounding nodes to line up with slider behavior. I'll make a branch later this week and a PR when it's got all the parts together.

@DigitalN8m4r3
Copy link
Contributor

So yes, please go ahead if your solution works better then mine, am happy with mine getting ditched in favor of something more robust

Thanks! it will be a breaking change but hopefully it ends up working better. The idea behind the custom slider is that the user can rotate the slider node with the Editor's rotation gizmos and using a 'private Transform3D' we save an origin point and rotation whenever there is a transform change from non-interactable-driven-behavior. Then the slider always slides 'back' from however its been rotated. So far I have it working for the slider and hinge.

The benefits are that there's no more SliderOrigin HingeOrigin nodes required, and the setup feels more intuitive from a user perspective, rotating the slider node into place rather than changing the surrounding nodes to line up with slider behavior. I'll make a branch later this week and a PR when it's got all the parts together.

cant wait to check it out, its possible that it will need readjustment after the following pr get merged #585

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants