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 setting for picking only top-most overlapping collision object #75688

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

mnemoli
Copy link
Contributor

@mnemoli mnemoli commented Apr 5, 2023

Fixes #29825

Personally I would prefer this to be default-on since it's much more intuitive and sensical for new users, but it's a change in behaviour and has a performance cost so better safe than sorry?

Sample project:
PickingTest.zip

@mnemoli mnemoli requested review from a team as code owners April 5, 2023 11:50
@YuriSizov YuriSizov added this to the 4.x milestone Apr 5, 2023
@Sauermann
Copy link
Contributor

Sauermann commented May 4, 2023

I had a look at the code and it looks like the changes implement the announced feature.

My personal preference would be to remove the interaction with physics_object_picking_sort by changing it the following way: The newly introduced viewport-flag would configure only, if the event is sent to all collision objects at the mouse location or if the event is sent to just the first encountered collision object. (The physics_object_picking_sort flag would configure if the first encountered collision object is the top-most collision object)

I am not sure if a project-setting for the root viewport is strictly necessary, since it can also be changed via GDScript.

@jsbeckr
Copy link

jsbeckr commented Aug 6, 2023

The current behavior is quite weird tbh and makes it necessary to use workarounds for any RTS style game. Is there an update on this issue?

@mnemoli
Copy link
Contributor Author

mnemoli commented Sep 23, 2023

Sorry for somewhat 'abandoning' this PR 🙂 @Sauermann My thoughts were it should be a project setting because the default behaviour, as jsbeckr points out, is weird and confusing - I think there should be high visibility and a low barrier to setting this for new users, especially since Viewport is a non-obvious place (imo) to go looking for physics flags.

I see your point about separating concerns by changing this to a "pick first encountered" flag (rather than "pick top") but find it hard to imagine a use-case where somebody would want to pick the first randomly encountered physics object so suspect both flags would be set by users in 99% of cases? Let me know if your thoughts have changed on either of these, if you're still of the same mind I'll make changes

e: github sync fork made a complete hash of the branch so I had to reset it, excuse the force push

@RyanGosden

This comment was marked as off-topic.

@AThousandShips
Copy link
Member

@RyanGosden Please don't bump things without contributing significant new information. Use the 👍 reaction button on the first post instead.

@Sauermann
Copy link
Contributor

a use-case where somebody would want to pick the first randomly encountered physics object

The use-case, that I can see is that it doesn't matter, which physics object is encountered and just a single one is wanted.

Let me know if your thoughts have changed on either of these, if you're still of the same mind I'll make changes

I personally would still prefer the approach of the separated concerns.

@mnemoli
Copy link
Contributor Author

mnemoli commented Jan 1, 2024

@Sauermann OK, have changed to "first only" rather than "top only" and separated from sorting. Have also removed from project settings since the intention there was to give new users a visible way to get an intuitive top-only picking setting. New test project attached with both flags off by default, enable them in scene.gd:_ready to test behaviour.

PickingTest2.zip

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

Here is a MRP to test the changes, which work fine:
BugPickingZSort.zip

Implementation makes sense to me.

Please squash the commits together as explained in the contribution guidline:
https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

Beside my comments, this PR looks ready.

doc/classes/Viewport.xml Show resolved Hide resolved
Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

Tested PR rebased on 4e990cd.
Works as expected.
Implementation is looking good.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 12, 2024
@akien-mga akien-mga merged commit 2b36dcf into godotengine:master Feb 12, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Anutrix
Copy link
Contributor

Anutrix commented Mar 16, 2024

Can we have some docs stating when/where(which scene or script or part of lifecycle) this can be enabled?

I currently enable it at _ready() of closest-to-root node's script. Hope this is the expected way to do this.

@AThousandShips
Copy link
Member

At any time? Are you facing issues with this at specific times? If so please open a bug report, if not why are you asking?

@Anutrix
Copy link
Contributor

Anutrix commented Mar 16, 2024

At any time? Are you facing issues with this at specific times? If so please open a bug report, if not why are you asking?

No issues yet. Both work great. Really happy with it.
Just lack of docs/example.

I just came across this while implementing a simple dragging behavior for Area2D objects.
Similarly, I believe new Godot users might be expecting behavior that physics_object_picking_first_only and physics_object_picking_sort provides and come looking here.
Including who haven't yet grasped/understood viewports.

Sharing it for them:

func _ready():
	get_viewport().set_physics_object_picking_sort(true)
	get_viewport().set_physics_object_picking_first_only(true) # Added in Godot 4.3

@AThousandShips
Copy link
Member

AThousandShips commented Mar 16, 2024

Nothing is missing, you can use it everywhere, and at any time, so stating that wouldn't help, and I don't think an example would be relevant either

If we were to state that nothing special is required everywhere the documentation would be flooded, if there's no notes about usage you should assume nothing special is needed :) that's basic logic

It's just a simple property and it only affects input so there's literally no example code you could add that wasn't just "this is how you set a property"

@jsbeckr
Copy link

jsbeckr commented Mar 17, 2024

I tried it out in 4.3-dev5 and it works for input_event(...). Just a quick question wouldn't it make sense to also work for mouse_entered()/mouse_exited() events. I updated the test project to visualize my point. After enabling both options the behavior feels quite weird (mouse_entered and mouse_exited are fired constantly when moving the mouse).

BugPickingZSort_mouse_events.zip

I think the mouse_* events should also adhere to the physics_object_picking_* options. Or am I overlooking something?

@AThousandShips
Copy link
Member

That would require a proposal as it's a separate system I would say

@mnemoli
Copy link
Contributor Author

mnemoli commented Mar 17, 2024

The weird thing of mouse_exited/mouse_entered firing constantly does seem like a bug with this PR though. I guess because breaking early interferes with later exit checking code (in _gui_update_mouse_over?) though can't say I fully comprehend it atm. Maybe needs to continue looping with a check on whether the event was already sent, rather than breaking out.

@Sauermann
Copy link
Contributor

@mnemoli do you have by chance a reproduction project, where mouse_exited/mouse_entered is firing constantly? If so, please report it as a separate bug.

@mnemoli
Copy link
Contributor Author

mnemoli commented Mar 18, 2024

@Sauermann

BugPickingZSort2.zip

I've extended jsbeckr's project above to add the shape_enter/exit and mouse motion events. If you wiggle the mouse around over two logos with sort+first_only off you get this:
image

With both settings on you get this, and clicking also fires spurious enter/exit events:
image

I think I see the problem here:

if (mm.is_valid() && mm->get_device() == InputEvent::DEVICE_ID_INTERNAL) {
send_event = false;
}

If send_event == false then the break at L860 is never called. Moving L859/860 out of the send_event check makes all the events work as expected.

I'll raise a new bug.

@TheEliteCoder1
Copy link

TheEliteCoder1 commented Aug 7, 2024

At any time? Are you facing issues with this at specific times? If so please open a bug report, if not why are you asking?

No issues yet. Both work great. Really happy with it. Just lack of docs/example.

I just came across this while implementing a simple dragging behavior for Area2D objects. Similarly, I believe new Godot users might be expecting behavior that physics_object_picking_first_only and physics_object_picking_sort provides and come looking here. Including who haven't yet grasped/understood viewports.

Sharing it for them:

func _ready():
	get_viewport().set_physics_object_picking_sort(true)
	get_viewport().set_physics_object_picking_first_only(true) # Added in Godot 4.3

I would like this set_physics_object_picking_first_only(true) in godot 3.5 ? Is there anyway to do this since I've been working on this game for 3 years and Upgrading to 4.3 would change alot and I am not willing to go through those changes. I am creating a level editor where each instanized object of a packedscene Is Selected With an Area2d that uses a CollisionPolygon2D to detect input_event(). I need to only select objects in the highest layer. Unfortunatley, that is not in 3.5. Please Help! It is essential for my level editor since I will be allowing players to create thier own levels, thank you.

My input event looks something like this:

func _on_MouseArea2D_input_event(viewport, event, shape_idx):
	if event is InputEventMouseButton:
		if event.is_pressed() and event.button_index == BUTTON_LEFT:
			if Globals.Editor_EditingLevel:
				if get_parent().get_parent().EDIT_MODE == "DELETE":
					get_parent().get_parent().undo_redo.create_action("Delete Object From Level")
					get_parent().get_parent().undo_redo.add_do_method(get_parent(), "remove_child", self)
					get_parent().get_parent().undo_redo.add_undo_method(get_parent(), "add_child", self)
					get_parent().get_parent().undo_redo.commit_action()
				elif "SELECT" in get_parent().get_parent().EDIT_MODE:
					get_parent().get_parent().undo_redo.create_action("Select Object In Level")
					get_parent().get_parent().undo_redo.add_do_method(self, "select")
					get_parent().get_parent().undo_redo.add_undo_method(self, "unselect")
					get_parent().get_parent().undo_redo.commit_action()

@Luke9389
Copy link

Luke9389 commented Aug 12, 2024

Edit: I'm on 4.3.beta.3 branch

Am I blind or is there no setting for this in the Project Settings window?

I see this in the demo from above:
project.godot

[physics]

common/enable_physics_object_picking_top_only=true

But I'm not finding the actual setting in the Project Settings window. I'm sure it would work to just hard code that into project.godot but surely there's a field for it that I'm just not seeing.... right?

Screenshot 2024-08-12 at 12 48 04 PM

@akien-mga
Copy link
Member

@Luke9389 The setting was removed following review. Instead, you should call the added method on the viewport:

get_viewport().set_physics_object_picking_first_only(true)

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

Successfully merging this pull request may close these issues.

Two overlapping Area2D / pickable CollisionObjects both receive input event
10 participants