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

Separates interactable updating from PlayerInteractionComponent to InteractionRayCast #173

Merged
merged 37 commits into from
Apr 30, 2024

Conversation

aronand
Copy link
Contributor

@aronand aronand commented Apr 20, 2024

This PR holds multiple changes, the most notable being:

  • Adds InteractionRayCast that is attached to the player's interaction RayCast3D
    • InteractionRayCast is responsible for updating the currently seen interactable, if any. This is done via signals that the PlayerInteractionComponent listens to.
      • The state itself only changes whenever a new interactable comes into view, or the current interactable goes out of view
  • Reorganizes UI prompt updates to only happen once, upon a state being changed
  • Refactors to PlayerInteractionComponent:
    • Unused/unnecessary variables removed
    • Getters and setters added for variables to which it makes sense (e.g. state change warrants emitting a signal)
    • Removes most direct calls to interaction_raycast, instead relying on the signals
  • Small simplifications + whitespace fixes where noticed (hide whitespace changes in GitHub to only focus on code changes)

Closes #170

Depends on #169

(Due to there being lots of atomic commits, it may make sense to squash the commits depending on how you wish the repo history to look)

aronand added 30 commits April 18, 2024 11:02
Simplifies PlayerInteractionComponent by merging input checks for interact and interact2 to a single block.
Removes RayCast3D.is_colliding() check and gets that information via RayCast3D.get_collider() (null if no collision)
Simplifies attempt_reload() by decreasing the level of nesting and by rewriting the reload logic:
The reload now loops through the item slots, looking for suitable ammo. When ammo is found, the entire
quantity is used instead of one by one.
InteractionRayCast emits a signal whenever an interactable enters or
leaves the player's view.
…ponent

InteractionRayCast already checks that collider is in the correct group.
Thus this check is redundant in the PlayerInteractionComponent.
Not all interactables are CogitoObjects
Fixes UI prompt issues caused by implementation of InteractionRaycast.
* freed objects are now handled by the raycast
* player interaction component forces an UI update via signals after interaction
Gets rid of final UI prompt doubling issues by forcing an UI update each frame
attempt_reload() is seemingly supposed to be called if the player isn't looking at an interactable. This however doesn't seem to have worked as intended, so removing the check completely for now.
Adds a setter that handles emitting signals whenever the value of interactable changes. This ensures that the signals are emitted no matter what.
Moving all UI updates to a central location until a better solution for handling the UI is implemented
When the user picks up ammo for their currently wielded wieldable, the wieldables UI gets updated.
Adds a setter for carried_object which handles signaling started_carrying whenever a not-null value is assigned
Prompts are again only (re)built after the player interacts with something.
@aronand aronand changed the title Separates interactable updating from PlayerControllerComponent to InteractionRayCast Separates interactable updating from PlayerInteractionComponent to InteractionRayCast Apr 20, 2024
@aronand
Copy link
Contributor Author

aronand commented Apr 21, 2024

Regarding the drop prompt, you can actually observe it being broken in main as well at the moment. I suspect that could be fixed by actually adding state to the UI, possibly Player_Hud_Manager, which would then manage its own visibility reactively.
EDIT: This is now fixed.

@aronand
Copy link
Contributor Author

aronand commented Apr 24, 2024

After creating a simplified version of the raycast for my own project I noticed that it and this are spamming the unseen signal. It's a really hard one to fix though, as is_instance_valid() will be true for both null and for a freed object (also null == freed object == true) => seemingly no easy way for an early exit.

Checking for Object.is_queued_for_deletion() also doesn't seem to help unfortunately. I'm possibly still missing something here, because in one of my attempts things seemed to work fine. In that one I had prints in different places which possibly slowed things down enough for everything to work as intended, and things broke when said prints were removed.

Thus this is currently an imperfect solution, as I haven't come up with a way to stop the signaling.

EDIT: Well, an hour more of testing and I've potentially found a solution in my own project:

# Handle freed objects.
# is_instance_valid() will be false for null and for freed objects, but only
# null will return 0 for typeof()
if not is_instance_valid(_interactable):
	if not typeof(_interactable) == 0:
		_interactable = null
		return

Still need to verify it works for Cogito as well.

@aronand
Copy link
Contributor Author

aronand commented Apr 24, 2024

A modified version of the simplified raycast is now in.

One oddity I still noticed is that at least in Cogito, the freed object handling sometimes gets triggered even when only quickly hovering the cursor over multiple interactables, but in practice this seems to have no significant impact.
(Doesn't seem to happen in my own project, hard to say for sure, but possibly there's some sort of a bug elsewhere in Cogito that causes it)
EDIT: This seems to have been caused by a null collider being an object null, which when assigned to be the _interactable will trigger freed object handling. Fixed (seemingly) by making collider a null type when:
collider == null and typeof(collider) != 0

@aronand
Copy link
Contributor Author

aronand commented Apr 26, 2024

The drop prompt hack is now removed as well. This is achieved by not emitting interactable signals while an object is being carried.

With that I'd say the PR is feature complete. Let me know if you spot any bugs I may have missed.

A collider is or can be a null object instead of a null type. If it's a null object, change it to null so that _interactable isn't treated as a freed object.
Copy link
Owner

@Phazorknight Phazorknight left a comment

Choose a reason for hiding this comment

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

Very nice. Cleans up quite a few lines.
Thank you!

@Phazorknight Phazorknight merged commit 490ebf2 into Phazorknight:main Apr 30, 2024
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.

Separate interactable detection from PlayerInteractionComponent
2 participants