Feature/direction dependent block attachment

[Intybyte]

Describe in detail what your pull request accomplishes

When detecting a craft it checks if it is a block like a ladder, lantern on an attachable block, if said blocks aren't attached to the craft, then don't add them to the craft.

Related issues:

Fix #430

Checklist

• Tested

[DerToaster98]

Uhm how attached does it have to be? Cause if it is hardcoded to be directly attached that is hurtful to creative designs. Shipbuilders often use the debug stick to get blocks into states that aren’t possible in survival (e.g.: levers). Also is there a whitelist or tag where one can limit the blocks affected by this? If no you should add that to let builders keep their creative freedom

[TylerS1066]

This does work with ladders, but does not work with torches or signs, both of which I would like to see added.

[Intybyte]

Uhm how attached does it have to be? Cause if it is hardcoded to be directly attached that is hurtful to creative designs. Shipbuilders often use the debug stick to get blocks into states that aren’t possible in survival (e.g.: levers). Also is there a whitelist or tag where one can limit the blocks affected by this? If no you should add that to let builders keep their creative freedom

1 block only, if you are worried stuff like this is still possible (i think that is what you are refering to)

[DerToaster98]

One smaller hint: You could check if the block is a instanceof Attachable, which also covers banners, ladders and so on, Also it extends Directional, so the cast and check is not necessary there. FaceAttachable is still needed for grindstone and levers.

Also check for Hangable which covers lanterns and mangrove sapplings and potentially other future blocks too.

https://hub.spigotmc.org/javadocs/bukkit/org/bukkit/material/Attachable.html
https://hub.spigotmc.org/javadocs/bukkit/org/bukkit/block/data/Hangable.html

Also i think you should include a per-craft blacklist to prevent blocks from getting this treatment in addition if you haven't added that already

Edit: Saw your todo notes just rn, ignore the part about Hangable if Hangable isn't a thing in 1.18

[Intybyte]

One smaller hint: You could check if the block is a instanceof Attachable, which also covers banners, ladders and so on, Also it extends Directional, so the cast and check is not necessary there. FaceAttachable is still needed for grindstone and levers.

Also check for Hangable which covers lanterns and mangrove sapplings and potentially other future blocks too.

https://hub.spigotmc.org/javadocs/bukkit/org/bukkit/material/Attachable.html https://hub.spigotmc.org/javadocs/bukkit/org/bukkit/block/data/Hangable.html

Also i think you should include a per-craft blacklist to prevent blocks from getting this treatment in addition if you haven't added that already

Edit: Saw your todo notes just rn, ignore the part about Hangable if Hangable isn't a thing in 1.18

It is better that we don't use MaterialData, it is very deprecated and not used anymore, tried using that in the past and the whole server lags just to reinitialize a whole section of ancient code, displays a warning like "If this is not an old plugin, this must be an error", basically starts speaking latin just to get your code to run.

The BlockData equivalent is only used by [HangingSign, Tripwire, TripwireHook], the last 2 aren't that important/used so making another cast just for one block, HangingSign, that isn't even used that much as normal signs are cheaper, I don't think it is worth the effort.

[DerToaster98]

One smaller hint: You could check if the block is a instanceof Attachable, which also covers banners, ladders and so on, Also it extends Directional, so the cast and check is not necessary there. FaceAttachable is still needed for grindstone and levers.
Also check for Hangable which covers lanterns and mangrove sapplings and potentially other future blocks too.
https://hub.spigotmc.org/javadocs/bukkit/org/bukkit/material/Attachable.html https://hub.spigotmc.org/javadocs/bukkit/org/bukkit/block/data/Hangable.html
Also i think you should include a per-craft blacklist to prevent blocks from getting this treatment in addition if you haven't added that already
Edit: Saw your todo notes just rn, ignore the part about Hangable if Hangable isn't a thing in 1.18

It is better that we don't use MaterialData, it is very deprecated and not used anymore, tried using that in the past and the whole server lags just to reinitialize a whole section of ancient code, displays a warning like "If this is not an old plugin, this must be an error", basically starts speaking latin just to get your code to run.

The BlockData equivalent is only used by [HangingSign, Tripwire, TripwireHook], the last 2 aren't that important/used so making another cast just for one block, HangingSign, that isn't even used that much as normal signs are cheaper, I don't think it is worth the effort.

Attachable isn't marked as deprecated as far as i can see, other classes in the same package are though. Yes, initializing that takes time, that's why if it is necessary you put that into the startup section of your plugin which gets called in a phase that takes long anyway, so it does not matter.
Arguing that blocks aren't important based on a crafting recipe isn't really logical. Movecraft is also used a lot on creative servers where that stuff is going to be used potentially since it is a neat alternative to sign + any block for it to attach to.

Anyway, TL;DR: I don't think hard checking for a list of blocks is a good idea. Usually the blocks that are attached to anything share one thing in their code which in bukkit is likely some interface

[DerToaster98]

Anyway, i stated my thoughts on here cause i thought it was necessary, might also leave now that i did that

[oh-noey]

Please read the discussion before making a comment, the plan is to make this configurable as an option on craft type files.

[CiaoCiaoBambina]

Please read the discussion before making a comment, the plan is to make this configurable as an option on craft type files.

Forgive me, but this was never explicitly mentioned other than in DerToaster's requests for it to be executed. I see a lack of confirmation of this in any instance.

[oh-noey]

I, the person reviewing the PR as a maintainer of the project, requested it (as I'm not an active maintainer, I only review things when I'm asked by other maintainers, which typically is only when the change is highly technical in nature or is in an area they're unfamiliar with, like in the case of detection):

Add a CraftType entry to allowlist this process based on materials, and another one to blanket enable the feature.

To be clear, I totally appreciate your dedication to ensuring we meet the needs of all players, and I appreciate how thorough your comment was. If you have any other things in mind that currently restrict creative players, make an issue on it - while I can't guarantee someone would pick it up, having that kind of unique insight on currently unknown feature gaps would definitely be valuable.

[CiaoCiaoBambina]

Must have missed that, apologies.

[TylerS1066]

Looks good to me, just have a few comments on the defaults and their location in the codebase.