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

More signals for teleporter function #513

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

Conversation

tcrass
Copy link

@tcrass tcrass commented Sep 2, 2023

Implementation of the new teleporter signals suggested in #499

One thing, though, I'm not sure about is that after merging my feature branch back into master there was one line (line 21, player_material = ExtResource("4")) missing from function_teleport.tscn. I don't think I touched the scene file, but I can't explain to myself how, when and where that line went, either.

@BastiaanOlij
Copy link
Member

I think the removal of the player material is fine, this probably is a result of the IDE overriding the player material when it was introduced, and now that it's reloaded everything finding out that the overridden material and the embedded material are the same and thus not needed.

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.

Looks good to me, the only thing I'm wondering about is whether it will be inconsistent in sending out signals at startup when it assigns default value. But I doubt that will be an issue in real life.

@Malcolmnixon Malcolmnixon added this to the 4.3.0 milestone Sep 6, 2023
@tcrass
Copy link
Author

tcrass commented Sep 19, 2023

Hi, I just saw that signalling has been re-designed for the pointer function. The pointer now only provides one single signal, featuring an enum telling what kind of pointer event happened. This is, in fact, very similar to something I initially also came up with for the transporter function, but which I decided against for sake of consistency with what back then was the pointer's way of doing things. What do you think -- wouldn't it now be appropriate to re-work transporter signalling to also provide only a single signal sending "meta-events"?

Cheers -- tcrass

@Malcolmnixon
Copy link
Collaborator

The pointer_event is useful because pointers need to encapsulate large amounts of information (what is pointing, who it's pointing at, what the event was, and what the 3D world location is of the contact). As such it pretty much has to be interpreted by code to translate all this information into something meaningful.

Changing the teleport signals to a single combined object prevents using the signal-connection inspector from wiring up discrete action. In cases where you want to just play a sound in reaction to teleport-activated or teleport-exited, or teleported; you would now require a script to break apart the single event and inspect the reason.

@tcrass
Copy link
Author

tcrass commented Sep 24, 2023

The pointer_event is useful because pointers need to encapsulate large amounts of information (what is pointing, who it's pointing at, what the event was, and what the 3D world location is of the contact). As such it pretty much has to be interpreted by code to translate all this information into something meaningful.

Changing the teleport signals to a single combined object prevents using the signal-connection inspector from wiring up discrete action. In cases where you want to just play a sound in reaction to teleport-activated or teleport-exited, or teleported; you would now require a script to break apart the single event and inspect the reason.

Four out of seven transporter events also provide information about "what" is hit and "where" it is hit -- and on the other hand, in my use-case, I'm only interested in whether the pointer enters or exits something, but I don't care what and where. So from my point of view, pointer and teleport eventing isn't that different at all...

Anyway, I've already re-worked the teleporter script to provide a single teleport event, similar to the pointer case. I'll submit another pull request for that, and you go ahead and just pick whatever you consider more suitable.

Cheers -- tcrass

@tcrass
Copy link
Author

tcrass commented Sep 24, 2023

Seems I still haven't understood how pull requests work on GitHub -- I expected a pull request to always refer to a specific commit, but I just realized that the latest changes I made in my fork's master branch got automatically added to this pull request. This is not what I intended, sorry... No I'm also unsure whether I should create pull requests from my master branch (after updating it to the latest state of your repository), or if it would be better to create individual feature branches for different pull requests.

Anyway, regarding the transporter event implementation: Just let me know if you prefer the "multiple events" or the "all-in-one event" version. In case of the former, I'll just revert the commit implementing the latter, ok?

Cheers -- tcrass

@Malcolmnixon
Copy link
Collaborator

nderstood how pull requests work on GitHub -- I expected a pull request to always refer to a specific commit, but I just realized that the latest changes I made in my fork's master branch got automatically added to this pull request. This is not what I intended, sorry... No I'm also unsure whether I should create pull requests from my master branch (after updating it to the latest state of your repository), or if it would be better to create individual feature branches for different pull requests.

Anyway, regarding the transporter event implementation: Just let me know if you prefer the "multiple events" or the "all-in-one event" version. In case of the former, I'll just revert the commit implementing the l

Yeah, the general approach is to make a branch in your forked repository for any one feature, and then construct the pull request from that branch. That way you can iterate and clean up the PR in isolation.

I think it's best to keep it as multiple events. For similar reasons I'm also going to add a few helper signals for #537.

@BastiaanOlij BastiaanOlij modified the milestones: 4.3.0, 4.4.0 Nov 28, 2023
@BastiaanOlij
Copy link
Member

This needs updating for the current version of XR Tools if we are going to merge this.

We are planning a brand new teleporter implementation for XR Tools 2 so may take this along.

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

Successfully merging this pull request may close these issues.

3 participants