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 incorrect formspec-tooltip doc, add detail in 'floodable' & 'on_flood' docs #5660

Merged
merged 2 commits into from Apr 27, 2017

Conversation

BluebirdGreycoat
Copy link
Contributor

The C++ source says it's incorrect. This updates the documentation to reflect that.

@SmallJoker
Copy link
Member

Are there any further trivial errors in the Lua API that should be fixed? It's nonsense to have a pull for each line, just merge them all together into one, as long it doesn't get too large.

@BluebirdGreycoat
Copy link
Contributor Author

@SmallJoker none that I have noticed so far. I'll remember to collect several at once, next time.

@paramat paramat added @ Documentation Bugfix 🐛 PRs that fix a bug Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines labels Apr 25, 2017
@paramat
Copy link
Contributor

paramat commented Apr 25, 2017

👍

@BluebirdGreycoat
Copy link
Contributor Author

I've added another commit re: floodable property for nodes.

@BluebirdGreycoat BluebirdGreycoat changed the title Fix incorrect formspec tooltip documentation Fix incorrect formspec-tooltip doc, add detail in 'floodable' & 'on_flood' docs Apr 26, 2017
@nerzhul
Copy link
Member

nerzhul commented Apr 26, 2017

@paramat can you check second commit too ?

@nerzhul nerzhul added this to the 0.4.16 milestone Apr 26, 2017
doc/lua_api.txt Outdated
floodable = false, -- If true, liquids flow into and replace this node
floodable = false, --[[
^ If true, liquids flow into and replace this node. However, liquids
^ cannot themselves be floodable. ]]
Copy link
Contributor

@paramat paramat Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Liquids can be made floodable, it's just a very bad idea. So this needs to be clearer.
If true, liquids flow into and replace this node
On a newline:
Warning: Making liquid nodes floodable causes an extreme load on waterflow processing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the engine ignores the floodable property if the node is also defined as a liquid? At least in my test I could not get water to flow into lava (after marking lava floodable). The on_flood callback wasn't getting called at all (tested by writing to chat inside the callback).

Edit: I think I performed the test incorrect, I'll redo it.
Edit2: test was performed correctly. Liquids indeed do not flood themselves or other liquids. Setting the floodable property for them seems to have no meaning. @paramat ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i may be wrong.

doc/lua_api.txt Outdated
@@ -4010,7 +4012,9 @@ Definition tables
^ node placement (i.e. schematics and VoxelManip) or air nodes. If
^ return true the node is not flooded, but on_flood callback will
^ most likely be called over and over again every liquid update
^ interval. Default: nil ]]
^ interval. Note: liquid-type nodes cannot themselves be floodable, else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a newline:
Warning: Making liquid nodes floodable causes an extreme load on waterflow processing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand you right, you're saying the engine does indeed honor the floodable property for nodes also defined as liquids? I did not observe behavior consistent with that, myself.

Sorry for the learner's question, but what does 'on a newline' mean? Should I replace what I wrote there, or put your addition underneath, or ...?

Copy link
Contributor

@paramat paramat Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that my suggested text should start on a new line instead of continuing from '^ interval. 'newline' means 'start text on a new line'.

Copy link
Contributor

@paramat paramat Apr 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the text 'else liquids would continously flood themselves and overload the update queue.' is possibly untrue.
Still a good idea to not set floodable on liquids, in case it is causing load without actually triggering 'on flood'.

@paramat
Copy link
Contributor

paramat commented Apr 26, 2017

So something different is needed like:
'Warning: Making a liquid node 'floodable' does not work and may cause problems'.
^ That on a new line in both places.

@BluebirdGreycoat
Copy link
Contributor Author

BluebirdGreycoat commented Apr 27, 2017

Ok, I understand now. Changes coming shortly.
Edit: commit rewritten. @paramat ?

The original documentation did not specify that liquids should not themselves be floodable. This is probably something that should be mentioned.
@paramat
Copy link
Contributor

paramat commented Apr 27, 2017

👍

1 similar comment
@sofar
Copy link
Contributor

sofar commented Apr 27, 2017

👍

@nerzhul nerzhul merged commit d130e1f into minetest:master Apr 27, 2017
@BluebirdGreycoat BluebirdGreycoat deleted the fix_incorrect_doc branch April 27, 2017 16:10
@nerzhul nerzhul added this to Done in Minetest 0.4.16 Apr 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 PRs that fix a bug @ Documentation Trivial The change is a trivial bug fix, documentation or maintenance change, as per the Git Guidelines >= Two approvals ✅ ✅
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants