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

Allow non-walkable nodes to destroy falling nodes #14771

Closed
wants to merge 4 commits into from

Conversation

Emojigit
Copy link
Contributor

@Emojigit Emojigit commented Jun 22, 2024

This PR allows non-walkable nodes to destroy falling nodes by porting commit a512866 of Tunneler's Abyss's fork. Nodes with group destroy_falling_node = 1 will not be displaced by falling nodes; instead, if falling nodes attempt to create a node here, they will be destroyed and dropped as items. If you play a certain Mojang game, this copies the behavior of torches destroying falling sands and gravel.

Starting from cherry-picking Tunneler's commit, I changed the group name from a weird nostomp to a more informative one, destroy_falling_node. Documentation and testing nodes are also added. Note to Tunneler's devs: The group name has to be changed after merging this (likely in 5.10).

Technically all nodes can destroy falling nodes, but this flag is only practically useful on non-walkable nodes, hence the name.

To do

This PR is Ready for Review. This code has already proven to work and is stable on Tunneler's Abyss.

How to test

Try to fall nodes onto the following nodes:

  1. testnodes:destroy_falling_node_0: Should be displaced
  2. testnodes:destroy_falling_node_1: Should destroy the falling node
  3. Any existing non-walking node (I use a light source): Should be displaced

This is my test footage (the signs are de facto game-independent so no worries):

Video_2024-06-22_22-05-25.mp4

@Emojigit Emojigit changed the title Allow walkable nodes to destroy falling nodes Allow non-walkable nodes to destroy falling nodes Jun 22, 2024
@sfan5 sfan5 added Feature ✨ PRs that add or enhance a feature @ Builtin labels Jun 22, 2024
@Zughy Zughy added the Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. label Jun 22, 2024
@cx384
Copy link
Contributor

cx384 commented Jun 22, 2024

In my opinion, this feature is too specific.
Generalizing falling nodes with callbacks would make more sense, such that you can implement your own behavior.
e.g. to combine nodes when one falls into another, or implement what this closed PR wanted: #14291

Also, node falling is implemented in builtin, so games can easily implement it by themselves if I'm not wrong.
Maybe it is a bad design decision to have such non-trivial things as part of the engine.

@appgurueu
Copy link
Contributor

appgurueu commented Jun 22, 2024

I agree with cx384. Dehardcoding via callbacks is also the approach I would prefer. In fact, I'm not sure whether the "callback granularity" offered here isn't even sufficient already:

cx384 is right that games can override the builtin falling node entity and implement this. Wouldn't overriding try_place with something that checks this condition, and if it is not met, executes the regular try_place work?

Maybe it is a bad design decision to have such non-trivial things as part of the engine.

I agree that the engine is getting heavily into game logic territory here. At least modders have the option to pretty much entirely replace falling node entity logic though, should they deem it necessary, or to just hook into or replace a few callbacks if that suffices for their use cases (which we should probably document a bit better, to guarantee we don't break it in the future).

@tenplus1
Copy link
Contributor

tenplus1 commented Jun 23, 2024

A simple addition that adds a new game mechanic for falling nodes, I like it :)

@Emojigit
Copy link
Contributor Author

I am going to close this PR. I will implement this feature on my server by copying codes instead.

@Emojigit Emojigit closed this Jun 23, 2024
@appgurueu
Copy link
Contributor

I am going to close this PR. I will implement this feature on my server by copying codes instead.

As said, I don't see a need to copy code. Why can't you just override try_place and first run your logic? Have you tried this?

@sfan5
Copy link
Member

sfan5 commented Jun 23, 2024

Wouldn't overriding try_place with something that checks this condition, and if it is not met, executes the regular try_place work?

Note that the falling entity is entirely undocumented and theoretically subject to change, so mods can't extend it safely.
We could expose and document it but I'm not sure if that's a good idea.

The alternative would be to add explicit on_falling_try_place or similar callbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Builtin Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants