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

Tnt: Improvements #862

Closed
wants to merge 17 commits into from

Conversation

Projects
None yet
8 participants
@sofar
Copy link
Member

sofar commented Feb 21, 2016

This PR is meant to create a working queue for outstanding TNT mod changes. There are currently a ton of outstanding changes and these are not getting any proper testing together, and had lots of merge conflicts.

  • Adds on_blast() return itemstack functionality
  • Adds particle blast effect
  • Ejects itemstacks in the air in several stacks, not just one stack
  • Add TNT API
  • Calculate and pass TNT intensity to on_blast() callback
  • Use VoxelManip

I've carefully stacked all these commits and rebased them. Especially the last 3 commits had lots of conflicts that needed resolving.

I've tested this with several rounds of TNT, and everything is functional and not crashing so far.

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Feb 21, 2016

@red-001 please help me test this and queue TNT-related changes together.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Feb 21, 2016

Thanks for doing this.
We should be cautious about using a voxelmanip, see #816

@ShadowNinja

This comment has been minimized.

Copy link
Member

ShadowNinja commented Feb 23, 2016

IIRC the issue with using an LVM was not destroying locked chests and the like.

@Ekdohibs Ekdohibs added this to the 0.4.14 milestone Mar 3, 2016

@paramat paramat removed this from the 0.4.14 milestone Mar 3, 2016

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 3, 2016

This probably can't be done in time for 0.4.14 as sofar has stated.

@paramat paramat added the WIP label Mar 6, 2016

@paramat paramat changed the title Tnt: Improvements [WIP] Tnt: Improvements Mar 6, 2016

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Mar 8, 2016

Rebased, spotted one error (fixed) and re-pushed.

Tested extensively and this is working just fine.

@ShadowNinja - I don't understand the concern about LVM's not destroying locked chests. We purposedly disabled that - all locked nodes are not destroyed to prevent griefing.

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Mar 8, 2016

@paramat as a whole, this seems more than well enough to go into 0.4.14

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 8, 2016

I'll review then.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 13, 2016

Commits 1, 5, 6 see line comments.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 13, 2016

Surely the 'on blast queue' will fail because many 'on blast' functions are simply 'do nothing' to protect the node. When using voxelmanip all nodes are removed, then afterwards the 'on blast' functions are called, where 'do nothing' is useless.

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Mar 13, 2016

Right, but it's still several orders faster than without the VManip, so taking the hit on running mostly empty functions isn't that bad.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 13, 2016

I mean nodes protected by 'on blast = do nothing' will it seems no longer be protected.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 13, 2016

Oops actuallly it seems ok.

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Mar 13, 2016

Yes, in destroy() the protection is taken account for.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 13, 2016

Yes if there is an 'on blast' function the node is not removed by the vm.

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Mar 13, 2016

https://youtu.be/QSLOp0afutA ... just for kicks :)

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 13, 2016

So, needs more review but this might be okay for 0.4.14.

@paramat paramat added this to the 0.4.14 milestone Mar 13, 2016

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Mar 13, 2016

One more spelling fix in the TNT API text.


for _, data in ipairs(on_blast_queue) do
local dist = math.max(1, vector.distance(data.pos, p))
local intensity = 1 / (dist * dist)

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Mar 15, 2016

Member

According to lua_api.txt 1 is mid-range of regular TNT. Either this should be changed or lua_api.txt should be updated.

This comment has been minimized.

Copy link
@red-001

red-001 Mar 22, 2016

Contributor

wouldn't local intensity = (radius / 3) / (dist * dist) be better?

for _, data in ipairs(on_blast_queue) do
local dist = math.max(1, vector.distance(data.pos, p))
local intensity = 1 / (dist * dist)
local node_drops = data.on_blast(data.pos, intensity)

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Mar 15, 2016

Member

The return value of on_blast isn't documented in lua_api.txt.

This comment has been minimized.

Copy link
@sofar

sofar Mar 19, 2016

Author Member

these 2 doc changes will come in a separate PR, for obvious reasons.

if def then
local node_drops = minetest.get_node_drops(def.name, "")
for _, item in ipairs(node_drops) do
add_drop(drops, item)
end
end
return c_air
end

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Mar 15, 2016

Member

This could be restructured a bit to reduce indentation and only check def once:

if not def then
    return c_air
elif def.flamable then
    return c_fire
else
    ...
end

This comment has been minimized.

Copy link
@sofar

sofar Mar 19, 2016

Author Member

check

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 28, 2016

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Mar 28, 2016

Interesting. How many TNT's did you explode?

TNT: Fix up nil derefs
I spotted two places where under stress (many explosions) luajit would
end up passing nil to these functions. I'm not entirely sure how,
but it seems good form to guard against it, which does make it
more robust. After this patch, I'm not able to crash the server. With
many explosions, it may still lag significantly, but always returns
in the end.
@tenplus1

This comment has been minimized.

Copy link
Contributor

tenplus1 commented Mar 29, 2016

Sofar: could you please add this on_construct function to the tnt:burning node as you have broken a feature added to the existing tnt mod:

on_construct = function(pos)
    minetest.sound_play("tnt_ignite", {pos = pos})
    minetest.get_node_timer(pos):start(4)
end,

...this also means that the 'start timer' and 'play sound' lines in the tnt:tnt node are no longer required.

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Mar 29, 2016

@tenplus1 well spotted, indeed that was lost.

However, the timeout isn't always 4 seconds (gunpowder makes it 1) so I don't want to needlessly have the timer set twice, so I'll change it slightly.

@tenplus1

This comment has been minimized.

Copy link
Contributor

tenplus1 commented Mar 29, 2016

You will need to move the timer back into the on_construct for tnt:tnt_burning unless you want to break previous mod function.

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Mar 29, 2016

@tenplus1 please test this last change. I can't until later today.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Mar 31, 2016

Still weeks to release so i think time to get this stable for release, keeping milestone for now.

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Mar 31, 2016

I've added one small tweak that limits actual entity speed after the explosion to 250m/s. That's already crazy fast, but you could get higher speeds otherwise. I've seen items hit the world edge otherwise.

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Apr 6, 2016

Small fix to accidental sound deletion from tnt ignite in 792f9fd.

sofar added some commits Mar 29, 2016

TNT: Move timer start to on_create() for burning nodes.
We add on_create() handlers for both burning TNT and burning
gunpowder. Because gunpowder will explode TNT in 1 second,
and not 4, we need to modify the 4 second timer after we
make the TNT burning. Other mods can now place burning TNT
that will by default explode after 4 seconds.
Limit entity speed to 250m/s.
Let's just call it "terminal" velocity.
Call nodeupdate on the entire blast zone
This will make sand and gravel blocks on top of TNT actually fly
in the air.

  https://youtu.be/4omndVZijLc
@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Apr 6, 2016

Re-added an accidentally removed sound (tnt ignite sound)

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 12, 2016

👍

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 13, 2016

For performance how about calling nodeupdate over 'radius' instead of 'radius * 1.5'? Is it necessary to nodeupdate within 'radius' if those nodes are destroyed?

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Apr 14, 2016

Now that nodeupdate is a lot (and I mean a huge lot) more efficient I wanted to make sure that we nodeupdate any nearby nodes. Maybe we shouldn't do 1.5x but (radius + 2) or something like that, but the environmental impact of stuff collapsing a bit further away will make using tnt in caves more fun (gravel roofs collapsing etc.)

@tenplus1

This comment has been minimized.

Copy link
Contributor

tenplus1 commented Apr 14, 2016

New changes working great, although missing texture "tnt_blast.png" error... Also tnt blasts tend not to hurt mobs from the redo mobs, would it be possible to have this inside of entity_physics instead:

        local damage = (4 / dist) * radius
        if obj:is_player() then
            obj:set_hp(obj:get_hp() - damage)
        else
            obj:punch(obj, 1.0, {
                full_punch_interval = 1.0,
                damage_groups = {fleshy = damage},
            }, nil)
        end
@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Apr 14, 2016

Yes, although I'm thinking that should be a separate PR as I have more planned changes to TNT. I'd like to get this merged first.

@tenplus1

This comment has been minimized.

Copy link
Contributor

tenplus1 commented Apr 14, 2016

No probs...

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 15, 2016

I'm okay with radius * 1.5, i keep forgettng default TNT has a small radiius, however ..

Is it necessary to nodeupdate within 'radius' if those nodes are destroyed?

MTGame devs are elusive currently so i think i'll merge this soon and not wait for re-review by sfan5.
@sofar shall we merge these commits as-is or do you want to squash a few to reduce the number?

@sofar

This comment has been minimized.

Copy link
Member Author

sofar commented Apr 16, 2016

@paramat I'd like to keep these separate. I've spent a lot of time making sure that between commits it's bisectable, so if we do encounter issues, it will be easier to track down when something broke. I realize it's a lot of commits, but each has lengthy and complete documentation as well, which would also be lost, or turn into a useless summary.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 16, 2016

That's fine.

Is it necessary to nodeupdate within 'radius' if those nodes are destroyed?

So we can deal with this in #1039 then. I'll merge this in a few hours.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Apr 16, 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.