Permalink
Browse files

Shortcut hit detection in WidgetGroup, since groups UI are normally s…

  • Loading branch information...
NathanSweet committed Oct 4, 2013
1 parent e410f90 commit f92f71656f306a60c68790f4b8f5e39432d2c59d
Showing with 7 additions and 0 deletions.
  1. +7 −0 gdx/src/com/badlogic/gdx/scenes/scene2d/ui/WidgetGroup.java
@@ -20,6 +20,7 @@
import com.badlogic.gdx.scenes.scene2d.Actor;
import com.badlogic.gdx.scenes.scene2d.Group;
import com.badlogic.gdx.scenes.scene2d.Stage;
+import com.badlogic.gdx.scenes.scene2d.Touchable;
import com.badlogic.gdx.scenes.scene2d.utils.Layout;
import com.badlogic.gdx.utils.SnapshotArray;
@@ -148,4 +149,10 @@ public void draw (SpriteBatch batch, float parentAlpha) {
validate();
super.draw(batch, parentAlpha);
}
+
+ public Actor hit (float x, float y, boolean touchable) {
+ if (touchable && getTouchable() == Touchable.disabled) return null;
+ if (x < 0 || x >= getWidth() || y < 0 || y >= getHeight()) return null;
+ return super.hit(x, y, touchable);
+ }
}

4 comments on commit f92f716

@gjroelofs

This comment has been minimized.

Show comment
Hide comment
@gjroelofs

gjroelofs Oct 7, 2013

Member

This Commit made us sad; as it broke pretty much any and all adhoc adding of groups to Tables, without the use of cells.

Our use case:

We have several Tables used with moderate sizes to display unit information.
We attached WidgetGroups to these tables (to use them as the base location) using addActor().
Any children in Table, which are not children of Cells are summarily ignored in the pack() call to set the size of the Table.
Which in turn, led to an afternoon wasted in finding out why all groups attached in such a manner did not receive any events anymore :-(

Although I agree with the general guideline of this commit
It might be important to take a look at Table then.

Member

gjroelofs replied Oct 7, 2013

This Commit made us sad; as it broke pretty much any and all adhoc adding of groups to Tables, without the use of cells.

Our use case:

We have several Tables used with moderate sizes to display unit information.
We attached WidgetGroups to these tables (to use them as the base location) using addActor().
Any children in Table, which are not children of Cells are summarily ignored in the pack() call to set the size of the Table.
Which in turn, led to an afternoon wasted in finding out why all groups attached in such a manner did not receive any events anymore :-(

Although I agree with the general guideline of this commit
It might be important to take a look at Table then.

@NathanSweet

This comment has been minimized.

Show comment
Hide comment
@NathanSweet

NathanSweet Oct 7, 2013

Member

Very sorry about that. :( I couldn't think of a scenario where someone would be using a WidgetGroup with actors outside the group's bounds. It seems you do it solely to make the actors relative to the WidgetGroup.

There is probably a way to short circuit like this without breaking things. Maybe WidgetGroup or Table can check and cache in layout() for the min/max bounds of any child actor. This could lead to strange behavior though, if actors are moved and layout() is not called.

I will revert the change for now since it isn't really a performance problem.
c7158ad

Member

NathanSweet replied Oct 7, 2013

Very sorry about that. :( I couldn't think of a scenario where someone would be using a WidgetGroup with actors outside the group's bounds. It seems you do it solely to make the actors relative to the WidgetGroup.

There is probably a way to short circuit like this without breaking things. Maybe WidgetGroup or Table can check and cache in layout() for the min/max bounds of any child actor. This could lead to strange behavior though, if actors are moved and layout() is not called.

I will revert the change for now since it isn't really a performance problem.
c7158ad

@gjroelofs

This comment has been minimized.

Show comment
Hide comment
@gjroelofs

gjroelofs Oct 7, 2013

Member

I wouldn't do that just yet. :-)
I actually tend to agree with creating order in the chaos, and we solved the issue at our end.

The message was just to ensure that other people who might have issues, look at this commit as a starting point for their fixes.
As for the afternoon; I have the team on SNAPSHOT for a reason: so we can at least help out a bit with development by spotting any early bugs (?). My apologies if my previous comment seemed harsh.

Member

gjroelofs replied Oct 7, 2013

I wouldn't do that just yet. :-)
I actually tend to agree with creating order in the chaos, and we solved the issue at our end.

The message was just to ensure that other people who might have issues, look at this commit as a starting point for their fixes.
As for the afternoon; I have the team on SNAPSHOT for a reason: so we can at least help out a bit with development by spotting any early bugs (?). My apologies if my previous comment seemed harsh.

@NathanSweet

This comment has been minimized.

Show comment
Hide comment
@NathanSweet

NathanSweet Oct 7, 2013

Member

Too late. :) The "fix" wasn't actually fixing something that hurt anyone, it was just trying to be more efficient. No problem reverting until we come up with something better.

We could add a boolean to Group, "hitUsesBounds" (or some good name). It defaults to false in Group and WidgetGroup makes it true.

Member

NathanSweet replied Oct 7, 2013

Too late. :) The "fix" wasn't actually fixing something that hurt anyone, it was just trying to be more efficient. No problem reverting until we come up with something better.

We could add a boolean to Group, "hitUsesBounds" (or some good name). It defaults to false in Group and WidgetGroup makes it true.

Please sign in to comment.