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

Big simplification of slabs combination #1111

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@sofar
Copy link
Member

commented May 24, 2016

Combine slabs if identical based on orientations using a simple
lookup table if the nodes are identical.

Otherwise relies on place_node() to place the node, which properly
handles rotation compared to adjacent nodes already, and can
orient based on look_dir as well.

Largely based on kilbith's #807 PR. Slab combining and place_node()
usage by sofar.

@sofar

This comment has been minimized.

Copy link
Member Author

commented May 24, 2016

Replaces #807

@sofar

This comment has been minimized.

Copy link
Member Author

commented May 24, 2016

Fixes #1109

@sofar

This comment has been minimized.

Copy link
Member Author

commented May 24, 2016

@everamzah @ShadowNinja @paramat @webdesigner97

Please test this, It was somewhat late when I wrote this but I think this is the proper solution with the minimal amount of code.

if n0_is_upside_down and p0.y + 1 ~= p1.y then
param2 = 20
-- else attempt to place node with proper param2
minetest.item_place_node(ItemStack(wield_item), clicker, pointed_thing, node.param2)

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs May 24, 2016

Member

So you can't place upside-down slabs if the node above hasn't a 20-23 param2?

This comment has been minimized.

Copy link
@sofar

sofar May 24, 2016

Author Member

You can place upside down slabs without issues. If you place any slab against the side of an already-upside down slab, it will also be upside-down.

You can also combine an upside down slab from below with another slab.

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs May 24, 2016

Member

How do you place the first upside-down slab, then?

This comment has been minimized.

Copy link
@sofar

sofar May 24, 2016

Author Member

Can't. Need screwdriver once.

This comment has been minimized.

Copy link
@sofar

sofar May 27, 2016

Author Member

Fixed in a push to allow upside down placement and placement against walls.

@webD97

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

It's working fine for me, this is the behaviour I expected. Thanks for your quick reaction this problem, now I hope we can successfully merge this.

@paramat paramat added the Bugfix label May 24, 2016

@ghost

This comment has been minimized.

Copy link

commented May 24, 2016

Tested and works as expected. The only concern left is that using set_node and item_place_node bypasses the register_on_placenode callbacks (such as lowering satiation in hunger mod), which is present when using on_place. I think this doesn't matter so much, though, if it is the case.

👍

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented May 24, 2016

Will be nice to have this.

@PilzAdam

This comment has been minimized.

Copy link
Contributor

commented May 27, 2016

👎 if it's not possible to place the first slab upside-down without screwdriver.

@sofar

This comment has been minimized.

Copy link
Member Author

commented May 27, 2016

@PilzAdam I'll see if I can add that.

Do you expect the player look direction to be the deciding factor for the slab orientation, or the orientation of the selected face?

@0-afflatus

This comment has been minimized.

Copy link

commented May 27, 2016

Do you expect the player look direction to be the deciding factor for the slab orientation, or the orientation of the selected face?

Personally, the orientation of the selected face.

@sofar

This comment has been minimized.

Copy link
Member Author

commented May 27, 2016

@PilzAdam added placement "to" a face.

If under the placement face is an existing slab, the slab is placed the same as the previously placed slab.

But if the node pointed under is something else (a normal node), then the slab will be oriented appropriately. This allows upside down placement, or placement against a wall. Without any screwdriver.

Tested and slab combining and extending slabs all work fine. Please give it a try.

@sofar

This comment has been minimized.

Copy link
Member Author

commented May 29, 2016

@PilzAdam, @Ekdohibs @ShadowNinja @paramat @sfan5 please review, all comments are addressed, including placing the first slab upside down without screwdriver.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented May 29, 2016

Hmm, I may have some problems with it:
slab on slab (for some reason it is not placing it properly)-> https://i.imgur.com/AohJDvy.png
stair on slab (not sure if related but trying to place stair on that slab from different angles, and it stands the same like this -> https://i.imgur.com/TXCEyTx.png

Edit: found lua error, hold on a sec:
Trying to place any kind of door between those slab-walls: https://i.imgur.com/tdnIbhG.png
Error:

2016-05-30 01:33:31: ERROR[Main]: ServerError: Lua: Runtime error from mod 'doors' in callback item_OnPlace(): ...minetest\bin\..\games\minetest_game\mods\stairs\init.lua:147: attempt to index local 'pointed_thing' (a nil value)
2016-05-30 01:33:31: ERROR[Main]: stack traceback:
2016-05-30 01:33:31: ERROR[Main]:   ...minetest\bin\..\games\minetest_game\mods\stairs\init.lua:147: in function <...minetest\bin\..\games\minetest_game\mods\stairs\init.lua:146>
@sofar

This comment has been minimized.

Copy link
Member Author

commented May 29, 2016

Hmm, I may have some problems with it: slab on slab (for some reason it is not placing it properly)-> https://i.imgur.com/AohJDvy.png

on purpose. You're placing an slab on an upside-down slab, so it copies the orientation.

I could try and make an exception for this, but it would result in very messy and lengthy code.

stair on slab (not sure if related

that's basically the same as the above issue.

Looking into the door problem atm.

@sofar

This comment has been minimized.

Copy link
Member Author

commented May 29, 2016

Edit: found lua error, hold on a sec: Trying to place any kind of door between those slab-walls: https://i.imgur.com/tdnIbhG.png

This is an error in the Door mod. Fixed by #1119

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

Why is this still not reviewed?

@sofar

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2016

I think the only thing remaining is "placing a slab on top of an upside-down slab". Right now the placement orientation copies the existing stair/slab, while it should just place it normal side up. I think it can be added, it's one more exception case but not too complex.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2016

@PilzAdam @Ekdohibs @paramat @sfan5

Please review, all comments in the thread are addressed and I think this is complete.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

Investigating... but right now I don't see what's happening there - what slab are you placing against?

@ghost

This comment has been minimized.

Copy link

commented Jun 26, 2016

Looks like mossy cobble. Is it touched by xdecor/workbench?
Edit: LOL I though that was an inner stair.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2016

@sofar @everamzah
Update 2: it seems mossycobble causes lua stop while using this PR, on default it just refuses to transform.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

Seems to crash without this patch too? #1163

@sofar

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

Ahhh no, #1163 is only an issue for this PR.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

What is happening is that mossy cobble slabs are not craftable. There is no recipe item defined for mossy cobble so it is impossible for the code to combine mossy cobble slabs due to the fact that it has no idea what to combine it to.

I'm going to make it so that mossy cobble slabs are just not combinable, since they're an exception case.

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2016

Ah yes, mossycobble slab problem is related to this PR. It is fixed for now.
I tested this PR and in most cases it was working nice, much simpler to place slabs and panels, very quick. There were some small cases were I put helper blocks to orient properly, but most of the time it just works. 👍

https://i.imgur.com/NYg5idc.png
https://i.imgur.com/Hkw0YQN.png

@sofar

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2016

👍 from myself.

@sofar sofar added the One approval label Jul 1, 2016

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2016

Can you make it work for stairs and full blocks as well? For example, after I rotated some brick block, further blocks should follow its rotation...

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

I feel that would be overcomplex.

@ghost

This comment has been minimized.

Copy link

commented Jul 26, 2016

It's really great! It's a lot faster, and more combinations are possible. Plus, right now there's this leftover slab that remains for a bit. This doesn't suffer from that.

@t4im

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2016

Plus, right now there's this leftover slab that remains for a bit.

That could simply be prevented by disabling placement prediction.
on_rightclick doesn't do that by default, but it can be disabled for on_place, too with one more line.

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

I guess this is a workaround for #1109 as the real issue there is an engine issue detailed in minetest/minetest#4357 Once that is fixed (it will take a while) this code could perhaps be simplified.

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

t4im's line comments need addressing.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

Right, thanks for the comments, I need to address them. Will make some time for this.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2016

Pushed a fix which addresses all the comments from @t4im (thanks).

I want to do a few quick tests on this - will get to that later today.

@t4im

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

The question why this is done in on_rightclick is still open.
I don't see what could be gained? It certainly has the potential to cause problems.

return itemstack
end
if minetest.is_protected(pos, player_name) and not
minetest.check_player_privs(clicker, "protection_bypass") then

This comment has been minimized.

Copy link
@t4im

t4im Aug 26, 2016

Contributor

You should use minetest.get_player_privs(player_name).protection_bypass instead in the future, if you just check one priv like this.
It is internally called by check_player_privs, too, which does some unnecessary set to list conversation, you don't need here.

check_player_privs also has a broken signature of 2nd return values (set expected and either string or list returned, which means you can't really use that anyway without lots of checking) and should probably be deprecated one day.

This comment has been minimized.

Copy link
@paramat

paramat Sep 10, 2016

Member

in the future

Why in the future, can this be corrected now?

This comment has been minimized.

Copy link
@t4im

t4im Sep 14, 2016

Contributor

Can, just didn't seem important enough to stale for it.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2016

Had to think about that for a bit, since it's been a while since I dove into this problem and patch myself.

If we'd rely on on_place entirely for slabs, placement prediction would place the second slab in the adjacent node, and so visually it would look very weird to begin with, since the new slab would be placed first, then the server would remove it and modify the target slab to a block.

I can't remember whether that was the only reason, though. Only thing left would be to convert it to on_place entirely and see whether it all still works.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 10, 2016

👎 Thinking on this i now would prefer this to be done in 'on place' so that extra code is not run on every rightclick performed in-game, even if 'on place' placement prediction makes something appear and disappear.

@sofar

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2016

I'll redo this and see if that can be done. The original code uses both as well, so when I wrote this patch originally I just didn't consider it to begin with. Give me some more time on this and I think I can work it out.

@sofar sofar added WIP and removed One approval labels Sep 10, 2016

@sofar

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2016

Updated to entirely use on_place(). Needs a little bit more testing.

All the basic behavior is the same, with one exception:

Due to this now entirely relying on on_place(), this means that slab combining only works if pointed_thing.above() is either empty or is buildable_to. This is because we can not avoid the mandatory check to see if pointed_thing.above() is empty. If it isn't empty, on_place() will never get called. (test case: place slab in bottom half of node. Place full dirt node above slab. Place slab pointing to top of slab placed in first step - this last step does nothing)

I find this somewhat awkward, but not prohibitive for the patch. Most people combing slabs will not run into this corner case.

@sofar sofar removed the WIP label Sep 12, 2016

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

(test case: place slab in bottom half of node. Place full dirt node above slab. Place slab pointing to top of slab placed in first step - this last step does nothing)

No problem i wouldn't expect this to work.

This makes some assumptons about how a player wants to build but i find how it behaves fairly intuitive and better than current behaviour. It's also very clever.

screenshot_20160913_005858

Just one thing, placing on 2 sides of a column produces horizontal and then vertical lines, perhaps better to always have horizontal? As horizontal is usually wanted more and this also matches the horizontal lines of the column sides. I'm not too bothered though, only if this is simple to do.

local n0 = minetest.get_node(p0)
local n1 = minetest.get_node(p1)
local param2 = 0
if wield_item == under.name then

This comment has been minimized.

Copy link
@paramat

paramat Sep 13, 2016

Member

How about
if under and wield_item == uunder.name then
in case an unknown node is 'under'? Pedantic i know but might avoid a crash (?).

This comment has been minimized.

Copy link
@sofar

sofar Sep 13, 2016

Author Member

no, it's entirely possible (pointing at an unknown node). So, I'll fix.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

Line comment.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

Otherwise looks good.

@t4im

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

This is because we can not avoid the mandatory check to see if pointed_thing.above() is empty. If it isn't empty, on_place() will never get called.

Ah, that explains a few things. That underlying behavior also prevents for example combining homedecor-style fences or handrails to corners or catwalks on placement, because you usually point pass the nodebox during placement at the nodes beneath them (Contrary to when placing slabs, so this is probably indeed a smaller issue here.)

Perhaps an item definition flag might help improve this in the future some time? To have on_place not ignore placement of certain items. (Although i don't think this needs to delay this PR)

Big simplification of slabs combination
Combine slabs if identical based on orientations using a simple lookup
table if the nodes are identical.

Otherwise relies on place_node() to place the node, which properly
handles rotation compared to adjacent nodes already, and can orient
based on look_dir as well.

Initial slabs placed are oriented based on (1) the orientation of
the pointed "face" (assumes nodes are cubic, of course), and uses
the player look direction to orient the node n/e/w/s if the slab
is horizontal or upside-down. If placed against a vertical face,
the slab is placed against the face without rotation around the axis
perpendicular to that vertical face. This allows upside down placement
and vertical placement without screwdriver.

If a slab is placed on top of an upside down slab, or below a normally
placed slab, the rotation is inverted so that no "floating" slab
is created.

Largely based on kilbith's #807 PR. Slab combining and place_node()
usage by sofar.

Since this relies entirely on `on_place` mechanics, this fails to
combine slabs into a plain node if the space *above* is occupied.
This is unavoidable due to the fact that on_place() happens after
the checks required to see if pointed_thing.above is empty or not.
@sofar

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2016

Fixed up one comment by @paramat.

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

👍

@paramat

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

@paramat paramat closed this Sep 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.