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

fix wrong behaviors when explode_center = false #2916

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fluxionary
Copy link
Contributor

@fluxionary fluxionary commented Jan 14, 2022

EDIT: I botched the description of the bug originally; included is a table highlighting the unexpected behaviors.

I am not entirely sure what the intended use of this feature is, but it is currently quite broken, at least in the use cases I see in the wild.

I see that it as added in #1844 to address #1825. I can't make sense of what exactly the issue there was. When using this (my) PR, I verified that "on_blast()" is being called for every node in the explosion, including the center.

The current behavior of tnt.boom, when triggered on a non-tnt node, is as follows:
boombugs

Here, "unblastable" means that the node defines onblast = function() end, i.e. that it should never be broken by explosions.

I also made a mod for making it easier to test the behavior of explosions:
https://github.com/fluxionary/minetest-boomstick

I might be missing something, but to me, it makes the most sense to just remove the feature.

@tenplus1
Copy link
Contributor

explode_center is needed when counting tnt blocks so that the radius of the explosion can be set properly, it isn't needed when tnt.boom is used on it's own for things like mobs and lucky blocks.

@fluxionary
Copy link
Contributor Author

Hm. I suppose that means my fix doubles the power of a single TNT block. I can resolve that by still starting the count at 1, and ignoring whatever's at the center.

@fluxionary
Copy link
Contributor Author

Instead, I just replaced the center node with air, if it is tnt.

I suppose I didn't notice this because math.floor(3 * math.pow(2, 1/3)) == 3.

@sfan5 sfan5 added the Bugfix label Jan 14, 2022
@Desour
Copy link
Member

Desour commented Jan 14, 2022

To clarify why this parameter exists:
tnt.boom used to be used just by tnt (or other explosive) nodes, so a node in the centre causes the explosion. This tnt node should not drop anything or do other on_boom stuff (like causing another explosion), and the explosion should make light => tnt is replaced with "tnt:boom" node.
People now wanted to explosions without a tnt node in the middle (eg. explosive projectiles from dungeon masters). Now replacing the centre node is not ok, we need drop items or call on_boom.

What exactly is the buggy behaviour that this PR tries to fix?

I see some problems with the PR as it is now, ie. the missing "tnt:boom" and the special case for the mtg tnt nodes (also note that if a projectile flies into a tnt node, both should explode).

@fluxionary
Copy link
Contributor Author

the explosion should make light => tnt is replaced with "tnt:boom" node.

Oh, that's the purpose of the tnt:boom node. Except that as things currently stand, the node has no chance to emit light, because it is always immediately replaced by air. It looks like nothing else in the mod even uses that node, so it's entirely purposeless in this PR as it stands. That certainly doesn't seem right to me.

I see some problems with the PR as it is now, ie. the missing "tnt:boom" and the special case for the mtg tnt nodes (also note that if a projectile flies into a tnt node, both should explode).

My proposal for tnt:boom: after the explosion, if the center node is air, replace that with tnt:boom, and set a timer to remove it on the next server tick.

What exactly is the buggy behaviour that this PR tries to fix?

Currently, there is no way to trigger an explosion without TNT that doesn't also remove nodes from protected areas. See the pseudocode above.

@fluxionary
Copy link
Contributor Author

Hm. Apparently I'm a bit confused; the issue is actually not what I was describing, which was the behavior with a botched modification of the tnt mod. There is still an issue. Give me a moment to change the description.

@fluxionary
Copy link
Contributor Author

I updated the description with a table that highlights the unexpected behaviors.

@sfan5
Copy link
Member

sfan5 commented Dec 24, 2023

Is this still applicable?
Reading through this I'm not immediately convinced that there's an actual bug.

@fluxionary
Copy link
Contributor Author

Is this still applicable? Reading through this I'm not immediately convinced that there's an actual bug.

absolutely, on your-land we've switched to using a fork of the tnt mod that i created.

@appgurueu
Copy link
Contributor

appgurueu commented Dec 28, 2023

I don't think we can remove this feature without breaking backwards compatibility - even if it's poorly designed - especially as it does see use in the wild (for the use case mentioned by Desour).

That said, your table shows some behaviors which are indeed bugs, and those should be fixed.

@fluxionary
Copy link
Contributor Author

I don't think we can remove this feature without breaking backwards compatibility

how so? with this PR, the code does the correct thing whether there's a TNT node at the center of the explosion or not, unless you think there's someone out there that wants to create an explosion around a TNT node without destroying it?

@appgurueu
Copy link
Contributor

how so? with this PR, the code does the correct thing whether there's a TNT node at the center of the explosion or not, unless you think there's someone out there that wants to create an explosion around a TNT node without destroying it?

Is this

also note that if a projectile flies into a tnt node, both should explode

handled correctly? Currently it looks like you'll first be setting the TNT node to air, so you won't explode it.

@fluxionary
Copy link
Contributor Author

fluxionary commented Dec 29, 2023

also note that if a projectile flies into a tnt node, both should explode

handled correctly? Currently it looks like you'll first be setting the TNT node to air, so you won't explode it.

oh, i finally understand the purpose of explode_center - without it, there's no way to indicate that the node at the center of the explosion should contribute to the size of the explosion. i've re-added it, though note that it doesn't make the explosion the "sum" of a TNT explosion and a projectile explosion, it only scales the radius of the projectile's explosion.

after re-adding it, there's still several problems that need to be addressed regarding explosions in protected areas, or of supposedly un-explodable nodes, when explode_center = false. i'll try to work out a different solution.

@fluxionary fluxionary changed the title remove explode_center from tnt [WIP] fix wrong behaviors when explode_center = false Dec 29, 2023
@sfan5 sfan5 added the WIP label Dec 29, 2023
@fluxionary fluxionary changed the title [WIP] fix wrong behaviors when explode_center = false fix wrong behaviors when explode_center = false Dec 29, 2023
@fluxionary
Copy link
Contributor Author

alright, i think it's good now, with no API change.

@sfan5 sfan5 removed the WIP label Dec 30, 2023
@sfan5 sfan5 self-requested a review April 13, 2024 11:33
@sfan5
Copy link
Member

sfan5 commented Aug 11, 2024

Sorry. I'm not motivated to review this after all.

@sfan5 sfan5 removed their request for review August 11, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants