Builtin item: Tidy up the code #4370

SmallJoker · 2016-07-28T11:41:36Z

This commit changes the behaviour of try_merge_with:

self collects all similar items around instead of splitting (and deleting)

t4im · 2016-07-29T14:21:07Z

If you already change assumed gravity, then you should probably get the value from movement_gravity, not hardcode it.

That won't account for realm specific overrides on players, but at least then it's consistent with server wide settings.

SmallJoker · 2016-07-29T16:04:41Z

I need more opinions how the behaviour of the builtin item should be. From my side it looks okay.

tenplus1 · 2016-08-06T18:12:52Z

I would adjust merge radius to 1.0, but everything else works a-ok that I'm using changes on Xanadu server.

SmallJoker · 2016-08-07T11:57:36Z

Comments for the last commit:
The age of an item will be reset when it merged with others. This makes sense because the total entity count decreased, so there's no problem in keeping one item longer.
Also changed the merge radius to 1m, this will collect more items and result in less items on the floor.

I'm glad to see that everything runs fine on Xanadu, this means there were no problems found with these changes.

SmallJoker · 2017-04-18T13:07:12Z

Rebased.
@sofar I tried using your modified wool code + the palette submitted by juhdanad but I have no clue how to reproduce #5407 (comment) 1) using current HEAD and 2) with this pull applied.

rubenwardy

haven't tested

rubenwardy · 2017-08-11T00:54:32Z

builtin/game/item_entity.lua

+		local count = math.min(stack:get_count(), max_count)
+		local size = 0.2 + 0.1 * (count / max_count)
+
+		if not core.registered_items[itemname] then


odd how the old code didn't take aliases into account

rubenwardy · 2017-08-11T00:55:48Z

builtin/game/item_entity.lua

+
+		local stack = ItemStack(entity.itemstring)
+		local name = stack:get_name()
+		if own_stack:get_name() ~= name


with use breaks after binary operators

rubenwardy · 2017-08-11T00:56:42Z

builtin/game/item_entity.lua

-		return false
+		-- Merge the remote stack into this one
+
+		local pos = object:getpos()


rubenwardy · 2017-08-11T00:56:50Z

builtin/game/item_entity.lua

+
+		local pos = object:getpos()
+		pos.y = pos.y + ((total_count - count) / max_count) * 0.15
+		self.object:moveto(pos)


rubenwardy · 2017-08-11T00:57:25Z

builtin/game/item_entity.lua

-			-- Don't infinetly fall into unloaded map
-			self.object:setvelocity({x = 0, y = 0, z = 0})
+
+		local pos = self.object:getpos()


get_pos()

rubenwardy · 2017-08-11T00:57:37Z

builtin/game/item_entity.lua

-			self.object:setvelocity({x = 0, y = 0, z = 0})
+
+		local pos = self.object:getpos()
+		local node = minetest.get_node({


core as in builtin

rubenwardy · 2017-08-11T00:57:54Z

builtin/game/item_entity.lua

+			return
+		end
+
+		self.object:setvelocity({x=0, y=0, z=0})


set_velocity

rubenwardy · 2017-08-11T00:58:04Z

builtin/game/item_entity.lua

+		})
+
+		if is_falling then
+			self.object:setacceleration({x = 0, y = -gravity, z = 0})


set_acceleration

rubenwardy · 2017-08-11T00:58:16Z

builtin/game/item_entity.lua

+
+		if is_falling then
+			self.object:setacceleration({x = 0, y = -gravity, z = 0})
+		else
 			self.object:setacceleration({x = 0, y = 0, z = 0})


set_acceleration

SmallJoker · 2017-08-11T12:11:28Z

Thanks for the review. Does it look right now?

rubenwardy

👍 needs testing however

paramat · 2017-08-23T19:08:27Z

builtin/game/item_entity.lua

+			self.object:set_acceleration({x = 0, y = 0, z = 0})
+		end
+
+		-- Collect the items around to merge with


Previously, merging is only attempted if the item has 'physical_state' true (in motion). The merging function is intensive so should not run for stationary ('physical_state' false) items, which are the majority of items present in a world.
Although i'm not too sure, this new code looks like it can run merging for stationary items?

This is not an issue, as this code part is only executed when the physical state changes. For stationary items this code part will not be executed at all.

paramat · 2017-08-23T19:15:16Z

I'm ok with the changes to 'try merge with'.

My fixing of items on ice (which may be added to this PR) means your changes to 'on step' cannot be done in the same way.
For example, now that we have sliding on ice, an item can be in motion ('physical_state' true) without falling, because it is supported by a node and is moving horizontally.

So, 'on step' needs to be copied from #6300 and then possibly developed from that code.
Apart from my line comment everything else seems ok as far as i can tell.

Use setting movement_gravity Reset age on merge Set merge radius to 1.0m

paramat · 2017-08-24T19:22:28Z

builtin/game/item_entity.lua

+				local slip_factor = 4.0 / (slippery + 4)
+				self.object:set_acceleration({
+					x = -vel.x * slip_factor,
+					y = 0,


It's possible that a walkable node is detected below before the item has hit it, so detecting a walkable node below should not set always set acceleration to zero. This will be difficult to notice visually but is incorrect physics.

The item is physical when it is in motion so that it uses it's collisionbox to stop moving vertically, then when vertical velocity becomes zero (we need to detect this, as in the previous code) we know it has hit a node and is supported by it and we can then set vertical motion to zero.

Seeting motion to zero when a walkable node below is detected may result in the item floating above or slightly buried in the node.

Right, I'm using the collisionbox height now to detect the node below to ensure that the item is correctly lying on the node.

paramat · 2017-08-24T19:32:07Z

builtin/game/item_entity.lua

+			local slippery = core.get_item_group(node.name, "slippery")
+			is_slippery = slippery ~= 0
+			if is_slippery then
+				is_physical = true


It's possible that a walkable node is detected below before the item has hit it, because we don't know the size and offset of the item's collisionbox (and these vary). If the node is not slippery then this code sets physical to false (no motion), which is wrong.

Abrupt stops of the item when a non-slippery node was detected is fixed now.

Yes i noticed floating.

paramat · 2017-08-24T20:28:46Z

Will retest.

paramat · 2017-08-24T20:34:22Z

Ok seems to work now so 👍

New code structure Use setting movement_gravity Reset age on merge Set merge radius to 1.0m

SmallJoker force-pushed the cleanup_builtin_item branch from 4582607 to a09e3eb Compare July 28, 2016 17:13

SmallJoker force-pushed the cleanup_builtin_item branch from 59e2231 to b45cad9 Compare July 29, 2016 16:03

SmallJoker force-pushed the cleanup_builtin_item branch 2 times, most recently from c626eeb to faca89f Compare July 29, 2016 18:58

SmallJoker force-pushed the cleanup_builtin_item branch 3 times, most recently from e12dab4 to 4908238 Compare August 7, 2016 11:52

sfan5 force-pushed the master branch from c90b63a to 09f1a0c Compare December 21, 2016 13:13

SmallJoker closed this Dec 24, 2016

SmallJoker reopened this Dec 24, 2016

SmallJoker force-pushed the cleanup_builtin_item branch from 4908238 to 7662d8f Compare March 17, 2017 20:00

SmallJoker mentioned this pull request Mar 18, 2017

Item drop entities getting stuck if increases hitbox size. #5409

Closed

SmallJoker force-pushed the cleanup_builtin_item branch 2 times, most recently from 0d2b423 to 1e3eb19 Compare April 18, 2017 13:00

SmallJoker added the @ Server / Client / Env. label May 4, 2017

SmallJoker added the Rebase needed The PR needs to be rebased by its author. label May 21, 2017

SmallJoker force-pushed the cleanup_builtin_item branch from 1e3eb19 to 881f736 Compare June 14, 2017 18:03

SmallJoker removed the Rebase needed The PR needs to be rebased by its author. label Jun 14, 2017

rubenwardy reviewed Aug 11, 2017

View reviewed changes

SmallJoker force-pushed the cleanup_builtin_item branch from 881f736 to 882b541 Compare August 11, 2017 12:09

rubenwardy approved these changes Aug 12, 2017

View reviewed changes

rubenwardy added Testing needed One approval ✅ ◻️ Testing needed labels Aug 12, 2017

rubenwardy removed Testing needed2 Testing needed labels Aug 13, 2017

SmallJoker mentioned this pull request Aug 23, 2017

Item entity: Fix and improve motion on slippery nodes #6300

Closed

paramat reviewed Aug 23, 2017

View reviewed changes

paramat added the WIP The PR is still being worked on by its author and not ready yet. label Aug 23, 2017

SmallJoker added 2 commits August 24, 2017 20:26

Builtin item: Clean up the code

8fe79d9

Use setting movement_gravity Reset age on merge Set merge radius to 1.0m

Builtin item: Re-apply slippery function

9be31c4

SmallJoker force-pushed the cleanup_builtin_item branch from 882b541 to 9be31c4 Compare August 24, 2017 18:27

SmallJoker removed the WIP The PR is still being worked on by its author and not ready yet. label Aug 24, 2017

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

paramat mentioned this pull request Aug 24, 2017

Item entities vibrate and bob on, or fall through, slippery nodes #6305

Closed

paramat reviewed Aug 24, 2017

View reviewed changes

paramat added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 24, 2017

Address comments: Fix sliding off nodes, properly collide with ground

ec647d9

SmallJoker merged commit d3f1743 into minetest:master Aug 24, 2017

SmallJoker deleted the cleanup_builtin_item branch December 14, 2017 15:56

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

Builtin item: Tidy up (minetest#4370)

5df0fdd

New code structure Use setting movement_gravity Reset age on merge Set merge radius to 1.0m

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

Builtin item: Tidy up (minetest#4370)

d980f49

New code structure Use setting movement_gravity Reset age on merge Set merge radius to 1.0m

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Builtin item: Tidy up the code #4370

Builtin item: Tidy up the code #4370

SmallJoker commented Jul 28, 2016 •

edited

t4im commented Jul 29, 2016

SmallJoker commented Jul 29, 2016

tenplus1 commented Aug 6, 2016

SmallJoker commented Aug 7, 2016

SmallJoker commented Apr 18, 2017

rubenwardy left a comment

rubenwardy Aug 11, 2017

rubenwardy Aug 11, 2017

rubenwardy Aug 11, 2017

rubenwardy Aug 11, 2017

rubenwardy Aug 11, 2017

rubenwardy Aug 11, 2017

rubenwardy Aug 11, 2017

rubenwardy Aug 11, 2017

rubenwardy Aug 11, 2017

SmallJoker commented Aug 11, 2017

rubenwardy left a comment

paramat Aug 23, 2017 •

edited

SmallJoker Aug 24, 2017

paramat Aug 24, 2017

paramat commented Aug 23, 2017 •

edited

paramat Aug 24, 2017 •

edited

paramat Aug 24, 2017

SmallJoker Aug 24, 2017

paramat Aug 24, 2017

paramat Aug 24, 2017 •

edited

SmallJoker Aug 24, 2017

paramat Aug 24, 2017

paramat commented Aug 24, 2017

paramat commented Aug 24, 2017 •

edited

Navigation Menu

Builtin item: Tidy up the code #4370

Builtin item: Tidy up the code #4370

Conversation

SmallJoker commented Jul 28, 2016 • edited

t4im commented Jul 29, 2016

SmallJoker commented Jul 29, 2016

tenplus1 commented Aug 6, 2016

SmallJoker commented Aug 7, 2016

SmallJoker commented Apr 18, 2017

rubenwardy left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmallJoker commented Aug 11, 2017

rubenwardy left a comment

Choose a reason for hiding this comment

paramat Aug 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paramat commented Aug 23, 2017 • edited

paramat Aug 24, 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 Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paramat commented Aug 24, 2017

paramat commented Aug 24, 2017 • edited

SmallJoker commented Jul 28, 2016 •

edited

paramat Aug 23, 2017 •

edited

paramat commented Aug 23, 2017 •

edited

paramat Aug 24, 2017 •

edited

paramat Aug 24, 2017 •

edited

paramat commented Aug 24, 2017 •

edited