Make entity selection and collision boxes independently settable #6218

stujones11 · 2017-08-06T16:25:32Z

Addresses #6199

This PR adds an optional selectionbox property to entities that overrides the collision box dimensions, otherwise used as the default. This is particularly useful for vehicle mods where the collision box often needs to be a different size or shape to the vehicle itself.

~~Note that 'zeroing' the selection box table will cause the visible selection box to default to the collision box dimensions, however,~~ a pointable property has also been included as a means to prevent selection.

This PR will also make entities more consistent with nodes in terms of available properties, IMO these ones in particular are even more important for entities.

SmallJoker · 2017-08-06T17:51:43Z

src/content_cao.cpp

@@ -1501,7 +1501,14 @@ void GenericCAO::processMessage(const std::string &data)
 	if (cmd == GENERIC_CMD_SET_PROPERTIES) {
 		m_prop = gob_read_set_properties(is);

-		m_selection_box = m_prop.collisionbox;
+		aabb3f no_selection = aabb3f(0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f);


This is an ugly workaround. Rather extend ??????Environment::getSelectedActiveObject to skip the objects that aren't pointable.

Okay, I will look at that, however, the 'zeroed' aabb box also denotes that no selection box has been specified. I did say that it wasn't ideal but I see no other way of knowing whether a selection box was set in the entity def.

Rather than make changes to ClientEnvironment::getSelectedActiveObject I think it would be better to include the pointable check in GenericCAO::getSelectionBox where it already returns false for invisible objects and is subsequently skipped by getSelectedActiveObject

SmallJoker · 2017-08-06T17:53:33Z

doc/lua_api.txt

@@ -3956,6 +3956,8 @@ Definition tables
        collide_with_objects = true, -- collide with other objects if physical = true
        weight = 5,
        collisionbox = {-0.5, 0.0, -0.5, 0.5, 1.0, 0.5},
+        selectionbox = {0, 0, 0, 0, 0, 0}, -- defaults to collision box dimensions


If it defaults to the collision box, then why is this table different from the other?

Because this is the default setting and what you will see if you dump the property table.

SmallJoker

See comments

.

stujones11 · 2017-08-06T22:06:06Z

@SmallJoker Cheers for the prompt review, I think I have now addressed the issues you raised. This PR is really quite trivial and it would be nice to see it merged before I start getting rebase conflicts ;-)

SmallJoker · 2017-08-07T08:27:19Z

src/content_cao.cpp

@@ -614,7 +614,7 @@ GenericCAO::~GenericCAO()
 bool GenericCAO::getSelectionBox(aabb3f *toset) const
 {
 	if (!m_prop.is_visible || !m_is_visible || m_is_local_player
-			|| getParent() != NULL){
+			|| !m_prop.pointable || getParent() != nullptr) {


getParent() does not return nullptr. Either revert this change to NULL or correct the function definitions and all lines where that function is checked against returning NULL. Never, ever mix these two.

I think it's better to keep the nullptr and modify getParent().

Okay, I will revert that last commit asap, seems like the safest option.

SmallJoker · 2017-08-07T08:28:26Z

src/object_properties.cpp

@@ -71,6 +73,9 @@ void ObjectProperties::serialize(std::ostream &os) const
 	writeF1000(os, weight);
 	writeV3F1000(os, collisionbox.MinEdge);
 	writeV3F1000(os, collisionbox.MaxEdge);
+	writeV3F1000(os, selectionbox.MinEdge);
+	writeV3F1000(os, selectionbox.MaxEdge);
+	writeU8(os, pointable);


needs version bump for backwards compatibility.

Should this be done as part of this PR? I have seen other new entity properties added without the need for that. I'm not too sure how to go about that, TBH.

This is only possible by adding the new values in this output-stream below the already existing ones. Otherwise, older clients will read garbage from the new "gap" you opened here.

Okay, that makes sense, are there any examples of how to bump the version, another PR perhaps? Or do you think it would be better just to move the new fields to the end?

KISS. Move the fields to the end.

paramat · 2017-08-08T01:29:54Z

This will be useful for my car mod, currently the selectionbox is set to the collisionbox which by necessity is buried inside the vehicle and barely visible.

stujones11 · 2017-08-08T16:44:49Z

I have made the requested changes and squashed the commits but am unable to actually test this right now. I will do this asap but given the changes, I think it should be fine.

SmallJoker · 2017-08-09T10:19:05Z

Tested using pointable = false (and = true in minetest_game/mods/player_api/api.lua L49.
Works fine, but only when the server uses this commit. For some reason there's no SerializationError in this case when reading outside the actual packet. As a result of this, pointable is always set to false when playing on older servers. I.e. none of the entities is selectable.
EDIT: How about sending the inverted case and reading it with pointable = !readU8(is);, so a zero defaults to true?

stujones11 · 2017-08-09T17:07:55Z

@SmallJoker Thanks for testing, I have done as you suggested and inverted the case. I have tested on an older server and everything seems to work as expected, however, I am wondering if backwards compatibility is really that important right now. I mean the current development builds are pretty much broken on older servers anyway.

stujones11 · 2017-08-12T20:31:48Z

@SmallJoker I noticed a slight inconsistency with this on the server-side while looking at the ray-casting issue, I hope you are still okay with it?

SmallJoker · 2017-08-13T13:23:35Z

@stujones11 I don't see any regression - If you didn't find any problems while testing either, I'm fine fine with the new change.

stujones11 · 2017-08-13T14:45:09Z

Thanks but I spotted a problem, the server-side properties are updated first therefore it's no use updating the m_prop client-side, the server does not see this. I have now moved the selection box default to the script api, this is actually much better since it avoids the need to compare aabb3f boxes, which I never did like doing. Tested and working.

stujones11 · 2017-08-13T15:56:43Z

Reworded documentation since now the selection box will be the same as the collision box if no selection box was specified in the entity definition. A 'zeroed' selection box will now behave just as expected but pointable = false should still be used to prevent selection.

stujones11 · 2017-08-13T17:47:33Z

I have now extended these properties to also work with players, however, this requires some minor additions to the MTG player_api to make them actually useable for modders. I am not entirely sure there is any real use-case, it just seemed easier to include it over trying to document the inconsistency.

Note this commit also incorporates the ray-casting fix I made here #6241 so that would no longer be applicable if this were to be merged first.

nerzhul · 2017-08-16T18:13:37Z

src/object_properties.cpp

@@ -98,6 +100,9 @@ void ObjectProperties::serialize(std::ostream &os) const
 	writeF1000(os, automatic_face_movement_max_rotation_per_sec);
 	os << serializeString(infotext);
 	os << serializeString(wield_item);
+	writeV3F1000(os, selectionbox.MinEdge);
+	writeV3F1000(os, selectionbox.MaxEdge);
+	writeU8(os, !pointable);


why invert the boolean ? please keep the right value

This was suggested by @SmallJoker from the emote, it seems @sfan5 agrees.

#6218 (comment)

317c6d6 can be omitted by the merging developer if backwards compatibility is not an issue.

since readU8 defaults to false if out-of-bounds this needs to be inverted to get pointable to be true if omitted

@sfan5 it's crazy and not very dev friendly, we should fix this later... i dismiss my request

indeed this should be fixed

This can be fixed later without breaking anything so is there any chance one of you could give a second approval on this? It's fairly trivial stuff but useful for modders and also fixes a ray-casting bug ;-)

Backwards compatibility is no concern at the moment as we have just merged 2 compatibility breaking commits, so just choose whatever is best without considering new-old server-client compatibility. I guess not inverting is best.
The 2nd commit could be left out on merge.

sfan5 explanation

stujones11 · 2017-08-23T15:33:10Z

Rebased and squashed into 2 commits. I have reverted the backwards compatibility hack but left the revert commit so the merging developer can decide whether to include it or not.

…etest#6218) * Make entity selection and collision boxes independently settable

SmallJoker reviewed Aug 6, 2017

View reviewed changes

SmallJoker added @ Script API Feature ✨ PRs that add or enhance a feature labels Aug 6, 2017

SmallJoker previously requested changes Aug 6, 2017

View reviewed changes

SmallJoker reviewed Aug 7, 2017

View reviewed changes

stujones11 force-pushed the selection-box branch 3 times, most recently from e4b413c to d0a1471 Compare August 8, 2017 16:41

SmallJoker approved these changes Aug 10, 2017

View reviewed changes

SmallJoker added the One approval ✅ ◻️ label Aug 10, 2017

stujones11 force-pushed the selection-box branch 2 times, most recently from 8f47678 to b199a64 Compare August 13, 2017 17:53

nerzhul previously requested changes Aug 16, 2017

View reviewed changes

stujones11 mentioned this pull request Aug 21, 2017

Do not add base position to player selection box #6241

Merged

stujones11 force-pushed the selection-box branch from c448822 to 5922ce3 Compare August 23, 2017 13:49

Make entity selection and collision boxes independently settable

1d9a69f

Revert "Invert pointable for backwards compatibility"

730820e

stujones11 force-pushed the selection-box branch from 5922ce3 to 730820e Compare August 23, 2017 15:29

nerzhul approved these changes Aug 24, 2017

View reviewed changes

nerzhul added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Aug 24, 2017

nerzhul merged commit ac4884c into minetest:master Aug 24, 2017

Wuzzy2 mentioned this pull request Nov 13, 2017

Item drop entities have a selection box Wuzzy2/MineClone2-Bugs#73

Open

stujones11 mentioned this pull request Dec 31, 2017

Use selection box for punch/rightclick entities and players #6865

Closed

stujones11 mentioned this pull request Feb 9, 2018

Position entity nametags relative to selection-box #7031

Merged

stujones11 deleted the selection-box branch February 9, 2018 19:50

osjc pushed a commit to osjc/minetest that referenced this pull request Jan 11, 2019

Make entity selection and collision boxes independently settable (min…

44a9784

…etest#6218) * Make entity selection and collision boxes independently settable

osjc pushed a commit to osjc/minetest that referenced this pull request Jan 23, 2019

Make entity selection and collision boxes independently settable (min…

c84be7d

…etest#6218) * Make entity selection and collision boxes independently settable

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make entity selection and collision boxes independently settable #6218

Make entity selection and collision boxes independently settable #6218

stujones11 commented Aug 6, 2017 •

edited

SmallJoker Aug 6, 2017 •

edited

stujones11 Aug 6, 2017

stujones11 Aug 6, 2017 •

edited

SmallJoker Aug 6, 2017

stujones11 Aug 6, 2017

SmallJoker left a comment

stujones11 commented Aug 6, 2017

SmallJoker Aug 7, 2017

Dumbeldor Aug 7, 2017

stujones11 Aug 7, 2017

SmallJoker Aug 7, 2017

stujones11 Aug 7, 2017 •

edited

SmallJoker Aug 7, 2017

stujones11 Aug 7, 2017

SmallJoker Aug 7, 2017

paramat commented Aug 8, 2017

stujones11 commented Aug 8, 2017 •

edited

SmallJoker commented Aug 9, 2017 •

edited

stujones11 commented Aug 9, 2017 •

edited

stujones11 commented Aug 12, 2017 •

edited

SmallJoker commented Aug 13, 2017

stujones11 commented Aug 13, 2017 •

edited

stujones11 commented Aug 13, 2017 •

edited

stujones11 commented Aug 13, 2017 •

edited

nerzhul Aug 16, 2017

stujones11 Aug 16, 2017 •

edited

sfan5 Aug 16, 2017

nerzhul Aug 16, 2017

sfan5 Aug 16, 2017

stujones11 Aug 16, 2017 •

edited

paramat Aug 17, 2017 •

edited

stujones11 commented Aug 23, 2017

Make entity selection and collision boxes independently settable #6218

Make entity selection and collision boxes independently settable #6218

Conversation

stujones11 commented Aug 6, 2017 • edited

SmallJoker Aug 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stujones11 Aug 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmallJoker left a comment

Choose a reason for hiding this comment

stujones11 commented Aug 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stujones11 Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paramat commented Aug 8, 2017

stujones11 commented Aug 8, 2017 • edited

SmallJoker commented Aug 9, 2017 • edited

stujones11 commented Aug 9, 2017 • edited

stujones11 commented Aug 12, 2017 • edited

SmallJoker commented Aug 13, 2017

stujones11 commented Aug 13, 2017 • edited

stujones11 commented Aug 13, 2017 • edited

stujones11 commented Aug 13, 2017 • edited

Choose a reason for hiding this comment

stujones11 Aug 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stujones11 Aug 16, 2017 • edited

Choose a reason for hiding this comment

paramat Aug 17, 2017 • edited

Choose a reason for hiding this comment

stujones11 commented Aug 23, 2017

stujones11 commented Aug 6, 2017 •

edited

SmallJoker Aug 6, 2017 •

edited

stujones11 Aug 6, 2017 •

edited

stujones11 Aug 7, 2017 •

edited

stujones11 commented Aug 8, 2017 •

edited

SmallJoker commented Aug 9, 2017 •

edited

stujones11 commented Aug 9, 2017 •

edited

stujones11 commented Aug 12, 2017 •

edited

stujones11 commented Aug 13, 2017 •

edited

stujones11 commented Aug 13, 2017 •

edited

stujones11 commented Aug 13, 2017 •

edited

stujones11 Aug 16, 2017 •

edited

stujones11 Aug 16, 2017 •

edited

paramat Aug 17, 2017 •

edited