-
Notifications
You must be signed in to change notification settings - Fork 575
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
Don't block fluid (water, lava) flow by flowers, mushrooms, grass and saplings #2942
Conversation
Why are you removing the |
I didn't removed flammable group, I just make it default to 1. |
Not quite a "default" as it overrides whatever is in the def but yes, I overlooked that. Sorry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine. PR title is misleading as the flowers are dropped, not destroyed.
|
Done
All flooded things are producing drops.
I would like to keep alghoritm simple and fast. Maybe it is not worth to implement that. Do you have example how it could be done? |
Performance impact is almost negligible. It might looks as follows: local cube_size = 0.6
local offset = (1 - cube_size) / 2
local function randomize_drop_pos(pos)
return vector.add(vector.add(vector.multiply(vector.new(math.random(), math.random(), math.random()), cube_size), offset), pos)
end or using signed random (perhaps more readable?): local cube_size = 0.6
local function sigrand()
return 2*math.random() - 1
end
local function randomize_drop_pos(pos)
return vector.add(vector.multiply(vector.new(sigrand(), sigrand(), sigrand()), cube_size), pos)
end |
What @Desour was saying is that when you generalize this to all MTG plant nodes, you should be determining the dropped item over the node def "drop" field using Which leaves me with an API suggestion: What about a default.def = {}
local function drop_on_flood(pos, oldnode, newnode)
for _, item in ipairs(minetest.get_node_drops(oldnode)) do
minetest.add_item(randomize_drop_pos(pos), item)
end
end
function default.def.floodable_drop(def)
def.floodable = true
def.on_flood = drop_on_flood
end which you can then simply use in |
Unfortunately after changing implementation to:
or
there is no more drops. It seems that it is not initialized here. |
You need to add the drops using |
Could you please test it with Lava and Water? |
Lava is interesting as it should both flood the crops and set them on fire; this might lead to a race condition if the ABM runs before
It is possible that |
From my testing it seems that crops are destroyed/dropped instantly, and there is no time for burn. |
What about griefers? Should there be a |
Unfortunately I am completely unexperienced with griefers. Could you please explain a little more how we should improve it? |
Say you have a protected area including a flower bed / mushroom farm / grass / tree farm (saplings). Griefers might now use liquids to destroy (flood) your beautiful farm as the liquids don't care about protection. A simple fix might be checking for protection, but the problem here is that liquids carry no owner/source information, which would make crops entirely unfloodable in protected areas... |
Ok, I get it. How I could set and test protected areas? |
Yes.
Agreed, but rather hard to implement with the current state of the engine.
You need a protection mod such as |
I have disabled randomization, as it looks weird. Instead I would like to implement moving dropping by water flow. I will do that in next Pull Request. |
I would not be worried about destroying flowers and grass with water by griefers. It could be easily avoided by putting some fence or wall etc. |
Please think it through first! A clean implementation definitely requires an engine change (to obtain liquid flow direction), a hacky one requires a significant amount of code checking all flowingliquid neighbors for each "flowing" item every step, which I imagine might eventually be performance critical. If you're determined to implement this, have a look at the following func to determine liquid flowing dir. Furthermore, this will require modifying the builtin item entity, which may be considered hacky as well (and should perhaps better be left to mods). |
Thanks for advice. You have right that |
Protection checks seem unnecessary (and also weird). |
Do you think it is ready to merge this PR? I have reached my personal goals with this implementation:
|
Please deduplicate the local Not a fan of the |
In previous implementation, little flower, grass, sapling or mushroom was barieer by which water or java was not able to pass. With this implementation water and lava is able to pass through flowers, grass, mushrooms and saplings by destroying it and also is allowing to pick up these items.
Fixed duplication of How many approvals need to be get to be able to merge PR? |
You need two approvals. |
It's unfortunate to have to ask this question now that the PR is finished but: Since Minetest Game is not accepting new features, shouldn't this be rejected? (#2710 for more detail) |
@sfan5 I think it is not a feature, as the flooding is working for example for torch and fire. It is just align the same behaviour with the rest of |
I renamed the title to show what is the real issue with water/lava flow. |
This was discussed in a coredev meeting we just had and the we weren't too sure either but the conclusion was that it is too close to being feature. |
In previous implementation, little flower or mushroom was barieer
by which water was not able to pass.
With this implementation water is able to pass through flowers
by destroying it and also is allowing to pick up these flowers.