-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add support for static boxes in 'leveled' type; add node box types 'leveled_plantlike[_rooted]' #11391
base: master
Are you sure you want to change the base?
Conversation
I've got a question: Would it be OK to allow selection boxes to be arbitrarily high? A leveled plantlike node ( So, can I drop the height limit safely and allow for selection boxes to go for the full plantlike height? Or is there still some edge case I might have overlooked? Dropping the hight limit would be really neat because this means you could make e.g. algae, kelps, etc. pointable everywhere, and that pixel-perfectly. |
b6fb24e
to
4466d49
Compare
I like all of these ideas to extend the functionality of param, param2, param3(?), |
Unfortunately, this feature is not on the roadmap and hasn't received a concept approval. Under the new rules, non-roadmap non-bugfix PRs have 7 days to be concept approved, otherwise they should be closed. These PRs can also have preapproval if they fix an issue that is concept approved If a core dev wants to support this PR, they may reopen it and apply the "Supported by core dev" label |
I totally understand that PRs can take months or even years to be even recognized by the core devs. I understand that time is a limited resource. I also understand when bad PRs are rejected for being bad. Because eventually, with enough patience, most of my PRs came through eventually. And those that were ultimately rejected were always rejected for a reason. Even if I didn't always agree with it, at least there was a reason. The long waiting times weren't great, but at least patience pays off eventually. But this new policy is just about murdering PRs without a single core dev having even looked at it. This is infuriating! There's no review, no consideration, no explanation, not even a single topic about the mere concept. Just a giant "Fuck you!", as if my work is not even taken serious anymore. Do you realize how condescending this looks? And no, I don't buy the excuse of "If a core dev wants to support this PR" ... It is closed now. Who is going to regularily look at the list of closed PRs? And a week seems short. So if all or most core devs are on vacation or distracted, all PRs posted in this period are by definition already doomed. Frankly, this new policy sucks. I am kinda pissed … But OK, I play along for now … According to the roadmap, I am supposed to post an issue before a PR. So I am going to do that … Among the >900 issues. Ooff. Fun times! I predict this will slow down development even further, because there are now two layers of approval (one for the issue, one for the PR), as if Minetest doesn't already have enough bureaucracy. ;-) Also: How on earth is "improving drawtypes" not on the roadmap? I am utterly confused. |
Well, I myself feel unhappy to let this PR get closed because it seems like a really good idea, but if I add a "Supported by core dev" label, then it seems as though I'm supposed to take a high level of responsibility for it. It seems to me that it's more likely for PRs to get closed than for core devs to take more on to their plate. But whatever. I'm not letting this get closed; I think Minetest's most important asset is nodes, so PRs to improve them are very good. I guess that basically makes me one forced reviewer. |
I have just rebased this PR to make it testable again. I repeat my question from before:
See #11391 (comment) |
I'd personally remove the limit. There's no limit to selection boxes (or collision boxes) for any other node, so it doesn't make much sense to enforce it for this type in specific. Of course, collision boxes break after a certain size, and even nodeboxes don't always display right at huge sizes, so if huge selection boxes break, it wouldn't be much of a surprise (or much of a problem either). |
Thanks for that. I am waiting for more opinions on this. In my tests so far, very large selectionboxes (if I disable the limit) still work fine. I'm mostly askin in case I overlook something. |
I'd like to express my support for this feature, but with some caveats. I am currently creating a "composter" node for MineClone2 and its derivatives and some aspects of this node box type might fit my purpose very well. However, it would not entirely fit my purpose, because I also need to have different textures depending on the node's level. AFAICS there is no support for having different textures mapped to varying levels in this PR. The composter is a static box that can be filled with 7 different levels of compost. It uses three different top textures for the node, ie. a "bottom" texture for the empty composter, a "compost" texture for the various stages of filling, and a "compost with bone meal" texture when the composter is ready for harvest. The solution that I am using now is to have several different node boxes defined each with a node attribute "_compost_level" and reading that from minetest.registered_nodes[node.name][_compost_level] . Seven nodes have almost identical definitions, apart from the node box's height and the name and _compost_level attribute that have been parametrized in the node registration. The "empty" and "ready" composters have additionally different textures defined. If I were to use the static leveled option that this PR implements, I could set the level of individual instances of composter nodes, but if I were to update the texture in the node definitions, this would affect the textures of all composter nodes placed on the map. Hence my additional feature request for some form of level-indexed node textures. Possibly, the original leveled type could even be extended to have indexed textures, that would IMHO add even more value to this PR. |
Please don't hijack PRs with major feature requests. |
Textures per nodebox (or per level) is something I've thought of myself before as there's no other way of doing that sort of thing without a model. However, it's not something for this PR, without a doubt. (Now I really need to review this PR...) |
Apologies if my remarks were taken as an intent to hijack the PR. I just wanted to describe a use case beyond the trivial and suggest a possible extension/enhancement. If that is beyond the scope of this PR then so be it. |
I did some extreme selection box testing. I tested selection boxes with heights up to 220 nodes. If anything would break selection boxes, the camera offset would. At a height of 220, it will 100% cross mapblock boundaries and camera offset boundaries. Surprisingly, selection boxes still seem to work perfectly. The bad news: selection boxes that big obliterated my framerate. I was getting perhaps 0.5-1 FPS. It doesn't matter whether the node exists on the map or not: just having a node definition with such a selection box causes it. Now, the highest selection box leveled plantlike_rooted could achieve (in this PR without the limit) is a height of just below 8. I didn't take any precise measurements, but there seemed to be very little difference in terms of framerate between Minetest with a selection box that big and Minetest without. In fact, I couldn't tell any difference up until after 14 or so. It might be that performance doesn't drop until selection boxes become bigger than mapblock sizes. Bearing all these things in mind, I feel like removing the limit would be reasonable. |
Thanks for testing. So I have increased the limit to 16.5, which is slightly above the maximum possible height of a It works fine for me, no issues or FPS drop. It also doesn't matter if the "plant" crosses mapblock boundaries, even if If you require any other changes or bugfixes, let me know! |
@Wuzzy2 please rebase |
Done. |
@Wuzzy2 aaaand rebase again.. |
The rebase has been completed. I also noticed the documentation was slightly wrong/outdated, so I fixed it. |
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.
Thanks for the PR.
Tested the three devtest test nodes, they work as expected - boxes match the visuals and keep matching when changing param2. Also tested with the box visualizer tool that the collision box is correct for the leveled fixed test node. The fixed boxes of e.g. stairs still work as expected.
I have some comments on the implementation, but nothing major.
(Also, rebased and fixed a compilation failure caused by a rename of a method you were using)
src/mapnode.cpp
Outdated
} else { | ||
box.MaxEdge.Y = (-0.5f + height / 16.0f) * BS; | ||
} | ||
if (box.MaxEdge.Y > SAFE_SELECTION_BOX_LIMIT * BS) { |
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.
Could use box.MaxEdge.Y = std::min(box.MaxEdge.Y, SAFE_SELECTION_BOX_LIMIT * BS)
.
Also, is the safe selection box limit really that large (~16 nodes)? This would seem unlikely to me, wouldn't raycasts have to be much slower if this was the case?
const std::vector<aabb3f> &to_add, enum NodeBoxType nbt) | ||
{ | ||
// Raw union (fixed) | ||
aabb3f half_processed(0, 0, 0, 0, 0, 0); |
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.
This seems slightly suspicious to me: What if you have a min point larger than 0, or a max point less than 0? I would initialize this box to {INFINITY, INFINITY, INFINITY, -INFINITY, -INFINITY, -INFINITY}
instead.
(That said, depending on how this union is used, it might not matter if it is a bit too large and includes the origin despite not having to.)
Sorry for the long wait, but I finally worked on addressing @appgurueu's comments. I changed the code and added some documentation clarification. There is no rebase yet (I'll wait for further feedback first). Here are my responses to the comments:
Oh crap. I haven't even thought of raycasts. This might ruin the entire PR because allowing high selection boxes is the main "selling point" of this PR. That feature is required to do what I want. I think the performance needs to be tested to see how large the impact is. (I wish we had raycasts that work on collisionboxes as well.)
Pretty much that. It's standard practice to always have the default case, even if it does nothing.
I think a switch would look messier in this instance.
No. This branch is about paramtype2, the previous conditional is about the nobebox type.
Your suggestion breaks the feature. But yeah, this might probably need more testing/investigation to be sure everything is OK. |
Good idea, let's talk a bit about this first.
Looking at the implementation, it should work correctly, since we use the bounds (i.e. how large a node can be) to search the area where relevant nodes may be.
Node size is a linear factor. If node boxes were at most 4 nodes tall before and now they can be 16 nodes tall, raycast performance - with respect to nodes - can be expected to decrease by roughly a factor of 4. If it was 2 nodes before (MTG doors) I would expect it to become 8 times slower if this feature is used. We should measure this however. I haven't tested this yet, but it should be testable with xcam. I don't think large selection or collision boxes for nodes are a particularly good idea. Even if used sparsely, our current architecture doesn't really allow dealing with them efficiently (or in a simple way, from a mod perspective). I would try to not stray too far from "1 node = 1 metric unit, cubed, or maybe a little less, but ideally no more". (With that in mind, have you considered a variant of this where large plants would consist of many nodes?) (Even smaller nodes like doors already create problems, since they break simplifying assumptions mods - and the engine - may make. For example: Can I now place a node overlapping with such a large plant? For water this may be desired, but what if I place, say, dirt?) Related: #14480
If you're thinking of the same use cases I'm thinking of, you would probably prefer something that lets you check for collisions with volumes, such as node boxes (to be placed), no? That's what I implemented in dbwts for example. Nowadays this could be implemented in much less code by leveraging grorp's new collision box getter API. (But I agree that we should probably have utilities akin to raycasts for this in the engine, for example to let modders check whether there is space to spawn an entity somewhere.)
Not very important, but I don't think that it is in any way a "good" standard practice to have a no-op default case. If the I try to keep in mind what happens if the code is changed (e.g. an enum value is added): Ideally, that should produce a compiler warning everywhere where it might matter. Explicitly specifying a If a compiler warning is not possible, it should fail loudly at runtime, rather than silencing the issue and doing something which may or may not be correct.
I took a closer look and this is used only by raycasts, to get an upper bound on selection boxes. I see no issue with always including the origin in that case. So we can scratch that point. |
Here's some context for this PR: The main reason why I added this PR is tall underwater plants like kelp and algae. Nodes that are But I understand the performance concerns. Leveled plants can get really tall. I'm looking forward to the performance test. If it turns out performance really goes to shit, I have an idea to "rescue" this PR: By limiting the selection box height to our current maximum. So smaller kelps still have a pixel-perfect selection box, but very tall kelps have their selection box capped at whatever is our current max height. This unfortunately breaks my vision for this PR somewhat, but it seems the best compromise to not break performance. Note that this PR only affects the Y size. X and Z are unaffected.
Ah yes, this is a good idea. |
The problems
Currently, 'leveled' boxes will allways have their upper face change with param2. So this only allows you to do fairly simple things. If you need a fully static box as part of a leveled nodebox, you are out of luck.
Also, you can't get selection boxes to be in sync with leveled plantlike nodes. No matter what you do, they are always off.
The PR
This PR adds a new field '
leveled_fixed
' for the nodebox type 'leveled'. This is an optional box (or boxes) for leveled-type nodeboxes, but it will be fully static.This screenshot shows an example (it's always the same node, but with different param2): https://satoshiupload.com/images/q5VZ1MoO8I.png
Moreover, it adds two new nodebox types
leveled_plantlike
andleveled_plantlike_rooted
. These work likeleveled
except they are in sync with the height of the leveled 'plant' for the drawtypesplantlike
andplantlike_rooted
, respectively. This allows you to make pixel-perfect selection boxes for such nodes.The height, however, is capped at one point because leveled plantlike nodes can get very high and I'm not sure how reliable selection boxes are when they're really high.
Fixes #8202.
Use cases
leveled_fixed
is the cauldron (which is static),fixed
is the water box (which varies in height))Please note
Unfortunately, the naming is a little bit weird for
leveled
nodeboxes. For some reason, thefixed
box is actually NOT fixed, since it varies in height. The new nodebox fieldleveled_fixed
is the box (or boxes) that is ACTUALLY fixed.It has been like that before, I cannot change that trivially w/o breaking compability. I think it is best to just keep the naming for the sake of compability, which is MUCH more important.
The nodebox types
leveled_plantlike
andleveled_plantlike_rooted
are intended to be used as selection boxes so you can capture the plant pixel-perfectly. New nodebox types were neccessary becauseleveled
has a step of 1/64 while leveled plantlike nodes have a step of 1/16. The new nodebox types use a step of 1/16 so it matches.leveled_plantlike_rooted
is basically the same asleveled_plantlike
but a height offset is applied (because of the base cube).Also, unlike
leveled
, the new nodebox types do NOT restart at param2=128.For collision boxes, they are only safe to use if you keep the upper end of the "plant" below a height of -0.5+127/64 (roughly 1.5), otherwise, collision bugs will start to occur. There is nothing in the code that enforces this limit so it's the responsibilty of the game author to make sure the "plant" doesn't become too tall. However, using these nodebox types as collision boxes is a rather esoteric use case anyway, so it should not be that big of a deal. I documented that anyway, just in case.
How to test
Use the following test nodes in DevTest. Try them together with the Param2 Tool.
testnodes:nodebox_leveled_fixed
(new node)testnodes:plantlike_leveled
(old node, new dynamic selection box)testnodes:plantlike_rooted_leveled
(old node, new dynamic selection box)