Update trapdoor climbable behavior #3938

Dehydrate6684 · 2023-09-16T07:36:20+02:00

Dehydrate6684 commented

2023-09-16 07:36:20 +02:00

Trapdoors are now only claimable below a ladder as stated in the TODO.

Not always climbable
Orientation

Relevant Wiki Entry: https://minecraft.fandom.com/wiki/Trapdoor#Properties

Testing

Place trapdoors below ladders and try climbing.

Trapdoors are now only claimable below a ladder as stated in the TODO. - [x] Not always climbable - [x] Orientation Relevant Wiki Entry: https://minecraft.fandom.com/wiki/Trapdoor#Properties ### Testing Place trapdoors below ladders and try climbing.

Dehydrate6684 added 1 commit 2023-09-16 07:36:21 +02:00

a960bf2e8e

Update trapdoor climbable behavior

Dehydrate6684 changed title from ~~Update trapdoor climbable behavior~~ to WIP: Update trapdoor climbable behavior

2023-09-16 08:24:45 +02:00

Dehydrate6684 added 1 commit 2023-09-17 11:12:01 +02:00

e6653b78ee

Added directional checks

Dehydrate6684 changed title from ~~WIP: Update trapdoor climbable behavior~~ to Update trapdoor climbable behavior

2023-09-17 11:12:31 +02:00

Dehydrate6684 added 1 commit 2023-09-17 11:14:13 +02:00

cc217d0089

Removed unnessary node definitions

chmodsayshello added the

controls

label 2023-09-29 17:12:01 +02:00

chmodsayshello requested changes 2023-09-29 17:52:47 +02:00

chmodsayshello left a comment

Some feedback I have after reading the code for the first time and testing for a few minutes

As of right now (on your branch), whenever placing a trapdoor, reagrdless of it's position, it gets placed as open, and closes right away, which is pretty annoying in my opinion, as it also plays the sound.
Also, it doesn't work with other climbable blocks, such as vines...
Hardcoding for a specific block like ladders isn't the way to go in this case I think, it should work with all climbable blocks no matter what.

In my test case, the trapdoors only became climbable in some very specific rotations, which I (personally) don't like, though I am not too sure if that is intended in this case

Some feedback I have after reading the code for the first time and testing for a few minutes As of right now (on your branch), whenever placing a trapdoor, reagrdless of it's position, it gets placed as open, and closes right away, which is pretty annoying in my opinion, as it also plays the sound. Also, it doesn't work with other climbable blocks, such as vines... Hardcoding for a specific block like ladders isn't the way to go in this case I think, it should work with all climbable blocks no matter what. In my test case, the trapdoors only became climbable in some very specific rotations, which I (personally) don't like, though I am not too sure if that is intended in this case

mods/ITEMS/mcl_core/nodes_climb.lua Outdated

					
				@ -14,0 +18,4 @@

				---@param trapdoor integer The param2 value of the trapdoor.

				---@return boolean If the ladder and trapdoor are in the same direction.

				local function check_direction(ladder, trapdoor)

					local convert_table = {};

chmodsayshello commented

2023-09-29 17:16:21 +02:00

Please out the hardcoded values in the array directly, like

local convert_table = {
    {1,23},
    {3, 21},
    ...
}

Also, please remove the semicolon, this isn't C ;)

Please out the hardcoded values in the array directly, like ``` local convert_table = { {1,23}, {3, 21}, ... } ``` Also, please remove the semicolon, this isn't C ;)

Dehydrate6684 marked this conversation as resolved

mods/ITEMS/mcl_core/nodes_climb.lua Outdated

					
				@ -14,0 +25,4 @@

					convert_table[5] = { 2, 22 }

					local conversion = convert_table[ladder];

					if conversion == nil then

chmodsayshello commented

2023-09-29 17:18:39 +02:00

Unless you have some special case where this should not be executes when the value = false (which it can't in this case, please use not

if not conversion then

Unless you have some special case where this should not be executes when the value = false (which it can't in this case, please use `not` `if not conversion then`

Dehydrate6684 marked this conversation as resolved

mods/ITEMS/mcl_core/nodes_climb.lua Outdated

					
				@ -14,0 +40,4 @@

				---@param ladder integer The param2 value of the ladder.

				---@param event "place" | "destruct" The place or destruct event.

				local function update_trapdoor(pos, ladder, event)

					local top_pos = vector.add(pos, { x = 0, y = 1, z = 0 })

chmodsayshello commented

2023-09-29 17:36:22 +02:00

This needs no change, but you could simply say { y = 1 }

This needs no change, but you could simply say `{ y = 1 }`

Dehydrate6684 commented

2023-10-01 10:32:08 +02:00

The others are needed otherwise it will cause the game to crash.

the-real-herowl commented

2023-10-09 12:32:27 +02:00

Could be vector.offset(pos, 0, 1, 0)

Could be `vector.offset(pos, 0, 1, 0)`

Dehydrate6684 marked this conversation as resolved

mods/ITEMS/mcl_core/nodes_climb.lua Outdated

					
				@ -14,0 +43,4 @@

					local top_pos = vector.add(pos, { x = 0, y = 1, z = 0 })

					local top_node = minetest.get_node_or_nil(top_pos)

					if top_node then

						local new_name = top_node.name

chmodsayshello commented

2023-09-29 17:39:16 +02:00

You may check if it's a trapdoor RIGHT here, and return if not, as otherwise, this won't do anything

Dehydrate6684 marked this conversation as resolved

mods/ITEMS/mcl_doors/api_trapdoors.lua Outdated

					
				@ -51,0 +54,4 @@

				---@param ladder integer The param2 value of the ladder.

				---@param trapdoor integer The param2 value of the trapdoor.

				---@return boolean If the ladder and trapdoor are in the same direction.

				function check_direction(ladder, trapdoor)

chmodsayshello commented

2023-09-29 17:27:12 +02:00

This seems like code duplication to me...
Also, when making something global, it should contain the modname, for example
mcl_doors.check_direction

This seems like code duplication to me... Also, when making something global, it should contain the modname, for example `mcl_doors.check_direction`

👍 1

Dehydrate6684 marked this conversation as resolved

mods/ITEMS/mcl_doors/api_trapdoors.lua Outdated

					
				@ -206,0 +248,4 @@

							fixed = {-0.5, -0.5, 5/16, 0.5, 0.5, 0.5}

						},

						on_rightclick = on_rightclick,

						mesecons = {effector = {

chmodsayshello commented

2023-09-29 17:33:17 +02:00

This one is nitpicky, but please fix the indention here

Dehydrate6684 marked this conversation as resolved

mods/ITEMS/mcl_doors/api_trapdoors.lua Outdated

					
				@ -206,0 +260,4 @@

					minetest.register_node(name.."_ladder", {

						drawtype = "nodebox",

						tiles = tiles_open,

						use_texture_alpha = minetest.features.use_texture_alpha_string_modes and "clip" or true,

chmodsayshello commented

2023-09-29 17:32:53 +02:00

Isn't this always true no matter what?

Dehydrate6684 commented

2023-10-01 10:08:29 +02:00

I copied from the code above. I am not sure what is should be so I just left it there.

Dehydrate6684 marked this conversation as resolved

Dehydrate6684 commented

2023-10-01 10:06:09 +02:00

As of right now (on your branch), whenever placing a trapdoor, reagrdless of it's position, it gets placed as open, and closes right away, which is pretty annoying in my opinion, as it also plays the sound.

I think this was a thing before my changes but I do agree it should be changed.

EDIT: I tested this but this doesn't seem to be the case for me.

Also, it doesn't work with other climbable blocks, such as vines...
Hardcoding for a specific block like ladders isn't the way to go in this case I think, it should work with all climbable blocks no matter what.

This is intended as in Minecraft only ladders have this kind of behavior with trapdoors. Hence, this property does not apply to vines, weeping vines and other climbable blocks.

In my test case, the trapdoors only became climbable in some very specific rotations, which I (personally) don't like, though I am not too sure if that is intended in this case

This is also intended behavior. The trap door only behaves like a ladder when has the same orientation (facing the same direction as the ladder).

> As of right now (on your branch), whenever placing a trapdoor, reagrdless of it's position, it gets placed as open, and closes right away, which is pretty annoying in my opinion, as it also plays the sound. I think this was a thing before my changes but I do agree it should be changed. EDIT: I tested this but this doesn't seem to be the case for me. > Also, it doesn't work with other climbable blocks, such as vines... > Hardcoding for a specific block like ladders isn't the way to go in this case I think, it should work with all climbable blocks no matter what. This is intended as in Minecraft only ladders have this kind of behavior with trapdoors. Hence, this property does not apply to vines, weeping vines and other climbable blocks. > In my test case, the trapdoors only became climbable in some very specific rotations, which I (personally) don't like, though I am not too sure if that is intended in this case This is also intended behavior. The trap door only behaves like a ladder when has the same orientation (facing the same direction as the ladder).

the-real-herowl commented

2023-10-07 10:19:21 +02:00

As of right now (on your branch), whenever placing a trapdoor, reagrdless of it's position, it gets placed as open, and closes right away, which is pretty annoying in my opinion, as it also plays the sound.

I think this was a thing before my changes but I do agree it should be changed.

EDIT: I tested this but this doesn't seem to be the case for me.

Also, it doesn't work with other climbable blocks, such as vines...
Hardcoding for a specific block like ladders isn't the way to go in this case I think, it should work with all climbable blocks no matter what.

This is intended as in Minecraft only ladders have this kind of behavior with trapdoors. Hence, this property does not apply to vines, weeping vines and other climbable blocks.

In my test case, the trapdoors only became climbable in some very specific rotations, which I (personally) don't like, though I am not too sure if that is intended in this case

This is also intended behavior. The trap door only behaves like a ladder when has the same orientation (facing the same direction as the ladder).

In my opinion, this should be implemented for all climbables, regardless of what MC does.

> > As of right now (on your branch), whenever placing a trapdoor, reagrdless of it's position, it gets placed as open, and closes right away, which is pretty annoying in my opinion, as it also plays the sound. > > I think this was a thing before my changes but I do agree it should be changed. > > EDIT: I tested this but this doesn't seem to be the case for me. > > > Also, it doesn't work with other climbable blocks, such as vines... > > Hardcoding for a specific block like ladders isn't the way to go in this case I think, it should work with all climbable blocks no matter what. > > This is intended as in Minecraft only ladders have this kind of behavior with trapdoors. Hence, this property does not apply to vines, weeping vines and other climbable blocks. > > > In my test case, the trapdoors only became climbable in some very specific rotations, which I (personally) don't like, though I am not too sure if that is intended in this case > > This is also intended behavior. The trap door only behaves like a ladder when has the same orientation (facing the same direction as the ladder). In my opinion, this should be implemented for all climbables, regardless of what MC does.

Dehydrate6684 added 1 commit 2023-10-09 07:58:34 +02:00

0c48a46f7c

Updated code based on reviews

Dehydrate6684 commented

2023-10-09 08:01:50 +02:00

In my opinion, this should be implemented for all climbables, regardless of what MC does.

I think we should create an issue tracker for this and put this in a different pull request. However, imo it might be weird to apply for all climbable because not all climbable are wall mounted.

> In my opinion, this should be implemented for all climbables, regardless of what MC does. I think we should create an issue tracker for this and put this in a different pull request. However, imo it might be weird to apply for all climbable because not all climbable are wall mounted.

the-real-herowl commented

2023-10-10 02:46:39 +02:00

In my opinion, this should be implemented for all climbables, regardless of what MC does.

I think we should create an issue tracker for this and put this in a different pull request. However, imo it might be weird to apply for all climbable because not all climbable are wall mounted.

Fine, then utilize the "ladder" group that you implemented.

Also, what is the reason to restrict the trapdoor to helping you climb only wall-mounted nodes? Logic? If you think about the logic of it though, why can't a trapdoor assist with climbing the climbable non-wall-mounted (centered) nodes? At this point, why would it be restricted to a single trapdoor? Can't you climb a stack of trapdoors (especially those with holes in their texture) like ladders? Speaking about ladders, you can place ladders on opposite or perpendicular surfaces and they're still climbable. Why can't a trapdoor be used the same way? Considering these, why change anything at all?

Anyway, go through the review you received and implement all the points. Mark all those you have already implemented as done, too.

> > In my opinion, this should be implemented for all climbables, regardless of what MC does. > > I think we should create an issue tracker for this and put this in a different pull request. However, imo it might be weird to apply for all climbable because not all climbable are wall mounted. Fine, then utilize the "ladder" group that you implemented. Also, what is the reason to restrict the trapdoor to helping you climb only wall-mounted nodes? Logic? If you think about the logic of it though, why can't a trapdoor assist with climbing the climbable non-wall-mounted (centered) nodes? At this point, why would it be restricted to a single trapdoor? Can't you climb a stack of trapdoors (especially those with holes in their texture) like ladders? Speaking about ladders, you can place ladders on opposite or perpendicular surfaces and they're still climbable. Why can't a trapdoor be used the same way? Considering these, why change anything at all? Anyway, go through the review you received and implement all the points. Mark all those you have already implemented as done, too.

Dehydrate6684 requested review from chmodsayshello 2023-10-10 03:29:38 +02:00

Dehydrate6684 added 1 commit 2023-10-10 03:40:02 +02:00

23468cc2dd

Used vector.offset instead of add/subtract

chmodsayshello reviewed 2023-10-16 19:58:26 +02:00

mods/ITEMS/mcl_core/nodes_climb.lua Outdated

					
				@ -14,0 +19,4 @@

				---@return boolean If the ladder and trapdoor are in the same direction.

				function mcl_core.check_direction(ladder, trapdoor)

					local convert_table = {

						nil,

chmodsayshello commented

2023-10-16 19:58:26 +02:00

why exactly do you have nil in a hard coded array?

Dehydrate6684 commented

2023-10-18 02:00:54 +02:00

This is due to the param2 of ladder. This way I can access the values I need without transforming the value for the array.

That being said probably could have minus one and it will still work.

This is due to the param2 of ladder. This way I can access the values I need without transforming the value for the array. That being said probably could have minus one and it will still work.

Dehydrate6684 marked this conversation as resolved

Dehydrate6684 added 1 commit 2023-10-18 02:05:23 +02:00

b320d008ca

Unhardcoded nil

Michieal commented

2023-10-18 03:15:25 +02:00

A simple question on this, and hopefully I don't sound ignorant in asking, but... Why not have a "climbable" group?
Ahh, you made a ladder group. hmmn.

A simple question on this, and hopefully I don't sound ignorant in asking, but... Why not have a "climbable" group? Ahh, you made a ladder group. hmmn.

Michieal commented

2023-10-18 03:19:50 +02:00

If you look at the way that minecraft does it, trapdoors are only climbable in certain instances. (I remember sometime last year asking about this.) I found it weird that the trapdoors used as shudders on the outside of buildings were able to be climbed.

Though, putting ladders one one side of a stack of trapdoors, is a cool way to have a ladder in the middle of a room. Though, in MC, the trapdoor side of that is not climbable.

If you look at the way that minecraft does it, trapdoors are only climbable in certain instances. (I remember sometime last year asking about this.) I found it weird that the trapdoors used as shudders on the outside of buildings were able to be climbed. Though, putting ladders one one side of a stack of trapdoors, is a cool way to have a ladder in the middle of a room. Though, in MC, the trapdoor side of that is not climbable.

the-real-herowl added this to the 0.85.0 - Fire and Stone milestone 2023-10-21 02:18:02 +02:00

the-real-herowl commented

2023-10-22 03:51:23 +02:00

I've got mixed feelings about this, because you can climb ladder configurations like the one in the attached screenshot, while you can't climb trapdoors with different orientation. I guess you could explain that ladders are more handy, but still...

Screenshot_20231022_034650.png

83 KiB

the-real-herowl requested changes 2023-10-22 03:54:42 +02:00

the-real-herowl left a comment

Add vine and skulk vein to the ladder group.

🚀 1

the-real-herowl requested changes 2023-10-22 04:10:02 +02:00

the-real-herowl left a comment

Remove the orientation requirement. So, if the trapdoor is perpendicular or opposite to the ladder, it should also be climbable. The rest of the constraints from the PR are accepted. If you implement this and the above request, we're merging this.

❤️ 1

the-real-herowl commented

2023-11-03 21:00:11 +01:00

How's it going? Are you going to finish it, or you would like somebody to pick this up? You could enable edits from maintainers if you want.

Dehydrate6684 added 2 commits 2023-11-05 07:11:39 +01:00

9b2b8ee56e

Removed direction checks

e5a260b563

Added vines and skulk vines to ladder group

Dehydrate6684 requested review from the-real-herowl 2023-11-05 07:12:03 +01:00

Dehydrate6684 commented

2023-11-05 07:12:45 +01:00

How's it going? Are you going to finish it, or you would like somebody to pick this up? You could enable edits from maintainers if you want.

Sorry I was a little busy lately but I manage to find some time and finish the requests.

> How's it going? Are you going to finish it, or you would like somebody to pick this up? You could enable edits from maintainers if you want. Sorry I was a little busy lately but I manage to find some time and finish the requests.

the-real-herowl approved these changes 2023-11-06 21:28:13 +01:00

the-real-herowl merged commit fdf823fff6 into master

2023-11-06 21:29:28 +01:00

the-real-herowl referenced this issue from a commit

2023-11-06 21:29:31 +01:00

Merge pull request 'Update trapdoor climbable behavior' (#3938) from Dehydrate6684/MineClone2:trapdoor-ladder into master

Sign in to join this conversation.

No reviewers