Make the player collisionbox settable #2738
Conversation
|
Does this fix #2612 ? |
src/content_cao.cpp
Outdated
| if (m_is_local_player) { | ||
| LocalPlayer *player = m_env->getLocalPlayer(); | ||
| aabb3f collisionbox = m_selection_box; | ||
| collisionbox.MinEdge += v3f(0,BS,0); |
|
No, this doesn't fix this problem. This just makes it possible to change the collision box for players like for entities. |
|
Why enlarge the inconsistence by also adding it (the offset) at this place? |
|
Because it is needed. Else the collisionbox doesn't fit to the player model. Fixing this would break compatibility with older clients. It should be fixed once we break network compatibility. |
|
Breaking network compat won't happen. What we could do however is to check the protocol version and then send (and recieve) correct values when the protocol is >= 25, and send/recieve "biased" values when the protocol is < 25. You can use this method on the serverside, and this field on the clientside. |
|
The real problem is the model, I just realized. It has this offset relative to models for mobs. It would break mods and games. |
|
Fixing this problem is impossible while staying compatible with older versions. It would need a fix in the model files. But on older clients, the collisionbox is fixed and thus the player would not fit inside the collisionbox but it would be halfway out. |
|
I think this could be a good addition to do for next minetest version. Add a boolean setting to minetestserver to set it and send it to player in second init phase is the best idea. Or maybe another packet dedicated to enable/disable this function as requested by server at each setting modification |
|
I don't get what you're saying. The function works as-is without sending additional data. The only problem is, it depends on an old hack. The player model doesn't stand on the ground but one node below the ground and thus, the player position is shifted. |
|
OK, as it seems the issue is larger. I guess this PR is not the place to fix the bug, or inconsistence. |
|
Thank you for creating this pull request! This fixes a limitation which heavily affects my Creatures mod, and I strongly welcome the change. As users might know, Creatures allows players to become various species (from mice to sheep to monsters to humans), and not being able to customize the collision box has been a huge limitation. The collision box is already being set for both players and mobs, but it just doesn't affect players. The player can already become a mouse for instance. Although they get the proper visual appearance and eye offset, they still have a huge inexplicable bounding box, and can't do things like crawling into 1m holes. Oppositely, if the player possesses a Dungeon Master, that player has an overly small collision box, and will clip through walls or the ceiling. Hopefully this puts an end to those problems, for Creatures and any other mod that does similar transformations to the player. |
|
I'm not an expert on this code but i support this feature. Is the implementation suitable for merging? |
Since you are a core developer you should know that, no? At least for something with that few lines. |
|
Not necessarily, some may understand everything, but i am a c++ newbie and many areas of Lua modding i know little about. Core developers are not required to understand everything. There are probably aspects of noise and mapgen you do not understand but i won't complain about your lack of knowledge. |
|
@paramat really knows alot about mapgen, I don't. |
Didn't say that, but this is nothing complex nor it is Lua API releated.I don't see anything in these 14 lines that would require special knowlege, as mapgens for example. Also, my point was not to blame you, it was ment as prompt to look at the code again. I actually think that you can know yourself whether this implemetation is suitable or not... |
|
Sorry i had another look and i cannot judge whether the code is correct, my ability with the core code is very patchy, can anyone else check it and +1? it's a nice feature. |
|
I know, the offset is hacky. But there is nothing that I can do about it. For the default player model, (0, 0, 0) is at the center (Well, 1 node above the ground) but for mobs (0, 0, 0) is at the ground level. Changing this would brake every mod, including Minetest Game. |
|
I documented the inconsistency, so at least modders know what's going on. |
doc/lua_api.txt
Outdated
| @@ -2921,7 +2921,8 @@ Definition tables | |||
| collisionbox = {-0.5,-0.5,-0.5, 0.5,0.5,0.5}, | |||
| visual = "cube"/"sprite"/"upright_sprite"/"mesh"/"wielditem", | |||
| visual_size = {x=1, y=1}, | |||
| mesh = "model", | |||
| mesh = "model", -- for mobs (0, 0, 0) is ground level, | |||
| -- for players (0, -1, 0) is ground level. | |||
There was a problem hiding this comment.
are these really the only two categories? It should be:
-- for players (0, -1, 0) is ground level,
-- for all other entities (0, 0, 0) is ground level.
|
+1 |
|
So what's the situation? Is this ever going to get pulled? |
|
Simpler than i expected. Would merging this cause anything we cannot reverse? If not i think the hacky offset is worth it. |
|
How would we reset the player collisionbox from a mod? |
paramat
added
Feature request
|
How bad would be to fix this properly in the player model? I am not aware of many other mods that change the player model besides my own 3d_armor and @MirceaKitsune's creatures. I would happily update my mod but it would probably best done around the time of a version update. |
|
The problem is the core game. To fix it properly, you would have to replace the player model in minetest_game. But that wouldn't be backward compatible, since older clients expect the model to have the offset. |
|
I think having the bbox settable by using the exact same property as on entities is the best approach. I am admittedly confused as to why doing something simple on players which works for entities / mobs would be such a complicated approach. |
Then perhaps this could be done for version 0.5.0? That is expected to break things, right? |
#2738 (comment) |
|
I don't understand what this has to do with network protocol. Does the mean we can never change the player model? |
|
Currently, the position of the players is in the middle of the model (10 units above the ground) and the position of other entities is on ground level. If we fix the model, we also have to change the network protocol to send the correct position. |
|
0.5.0 is going to break things. Maybe not necessarily break network compatibility, but it will break things. That's the whole point of changing from 0.4.16 to 0.5.0. If nothing was going to break, it would change to 0.4.17. I think we should solve it now with the hacky way, and once we close in on the 0.5.0 release, change to the better way. Just my opinion. |
|
TBH, looking at the code changes, I've seen a lot worse 'hacks' than this get merged. I still think the inconsistency should be fixed at the first opportunity. |
|
This PR prevented players walking through 2 node high spaces, something probably easy to miss in testing. However, it was not tested by devs so should not have been merged. Reopen as this is being reverted, we need to consider the correct implementation. |
|
As player collision box is hardcoded client side, we can now make this really work for everybody in 0.5 breaking changes |
|
Good point, a good opportunity. |
SmallJoker
added
Feature ✨
|
#6007 merged. |
It's not ideal since the collisionbox of players is off by 1. But fixing this problem would be incompatible with previous minetest versions, mods and games.