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

unmovable property for entities #15751

Open
mruncreative opened this issue Feb 3, 2025 · 16 comments
Open

unmovable property for entities #15751

mruncreative opened this issue Feb 3, 2025 · 16 comments
Labels
Feature request Issues that request the addition or enhancement of a feature @ Script API

Comments

@mruncreative
Copy link

mruncreative commented Feb 3, 2025

Problem

Many things do not want to be moved but unintentionally get moved. Often this is because entities represent functionality (see my target node) or graphics (see the cannon in Traverse) for nodes they can not represent.
A constructor and a destructor looking for an entity at the position of the node works fine but if the entity gets moved the node can end up without an entity.

Typically these entities don't react well to being teleported or given velocity. In fact I don't really want them to care or constantly do stuff. Entities exist to be movable. Not being movable breaks a core assumption about them.

Solutions

Add unmovable (boolean) to the initial property list for entities.
If present, set_pos, set/add_velocity and set/add_acceleration functions should do nothing when called.
Mods that repeatedly do heavy calculations before moving an object from a certain position can simply get the properties and check for unmovable to avoid them.

Alternatives

  1. Try to communicate the unmovablity to every single other mod and hope they don't fucking move it like I tried to do. I forked a bow mod to prevent it from adding velocity to random entities it hits. I called mesecon.register_mvps_unmov(objectname) and added it as mesecons_mvps optional dependency and now Mineclonia did it's own redstone thing and doesn't respect that anymore. I can add that but there are more things out there that don't know or care. The community didn't manage to agree on one way to signal unmovability and now it's fucked.

  2. Hijack the metatable and do what I described with turning functions into nullops in Lua but that's probably slow and not easy to detect for other mods which is why I'm proposing this.

Additional context

A demonstration of what things love to be unmovable:

mineclonia$ grep -r _mcl_pistons_unmovable
mods/ENTITIES/mcl_paintings/init.lua:		_mcl_pistons_unmovable = true
mods/ITEMS/mcl_chests/init.lua:	_mcl_pistons_unmovable = true
mods/ITEMS/mcl_itemframes/init.lua:	_mcl_pistons_unmovable = true
mods/ITEMS/mcl_itemframes/init.lua:	_mcl_pistons_unmovable = true,
mods/ITEMS/mcl_enchanting/init.lua:	_mcl_pistons_unmovable = true
mods/ITEMS/mcl_end/end_crystal.lua:	_mcl_pistons_unmovable = true
mods/ITEMS/mcl_banners/init.lua:	_mcl_pistons_unmovable = true
mods/ITEMS/mcl_mobspawners/init.lua:	_mcl_pistons_unmovable = true
mods/ITEMS/REDSTONE/mcl_pistons/api.lua:
if (entity or player) and not (entity and
minetest.registered_entities[entity.name]._mcl_pistons_unmovable) then
mods/ITEMS/mcl_signs/init.lua:	_mcl_pistons_unmovable = true
mods/ITEMS/mcl_armor_stand/init.lua:		_mcl_pistons_unmovable = true,

We see: the lid of an opened chest node (I believe), the book hovering on the enchanting table node, the rotating animation in the mob spawner, the sign text, the armour stand which has a node that destroys the entity displaying the armor when it gets pushed by a piston.

None of these things are exclusive to pistons. I've seen many mods that do move random entities and this would totally break everything, others confine themselves to certain entities and some use some sort of badly maintained list of which should be movable or not like Mesecons and Mineclonia.

@mruncreative mruncreative added the Feature request Issues that request the addition or enhancement of a feature label Feb 3, 2025
@mruncreative
Copy link
Author

mruncreative commented Feb 3, 2025

I'm not sure about the bouncing animation of the enchanting table book though. Is it implemented as an animation or is it moving?

@appgurueu
Copy link
Contributor

appgurueu commented Feb 3, 2025

Discussion on IRC: https://irc.luanti.org/luanti/2025-02-03#i_6239885.

Add unmovable (boolean) to the initial property list for entities.

Just bloating things is not necessarily the right approach (and I'm not convinced object properties would be the right place for this). Ultimately I think velocity, acceleration, and positional changes should be some form of "optional" "component", such that entities which don't need them don't have them at all (which also lets us optimize some things), not that they have both the boolean flag and the methods and properties (which then make no sense). See also #14751.

Note also the "physical" object property which controls collisions. Graphical entities will have this set to false. Mods that implement physical things (explosions, arrows, etc.) should probably leave entities that have it set alone.

If present, set_pos, set/add_velocity and set/add_acceleration functions should do nothing when called.

This is too hacky to be implemented by the engine 1. Clear 👎 from me. When a mod calls these functions, it expects them to do something. We should not cover up this false assumption. We don't know what the right thing to do is in general. Example: A mod that transplants a region of the world to somewhere else. Suddenly some entities just arbitrarily get left behind. Makes no sense.

Try to communicate the unmovablity to every single other mod and hope they don't fucking move it like I tried to do.

Yes, this is the proper solution. Mods have to consider whether an object should be moved before they moved it. A TNT mod will have to check which objects should be blasted, or destroyed, or whatnot (and usually you will still have to take some action for the static objects; "just do nothing" simply results in wrong behavior, such as entities floating mid-air, which may then be covered up by more hacks). We will not try to fix mods from under the modder explicitly asking the engine to do the wrong thing by introducing dirty hacks.

The community didn't manage to agree on one way to signal unmovability and now it's fucked.

That sucks. But fucking the engine is not the solution.

As I've said before: Games can do this properly. Mods can do this properly. They aren't. It's not the engine's job to clean up every mess (at the expense of making a mess of our own APIs no less). We've got enough pure engine messes to clean up, enough places where even a game developer in full control of the entire codebase can't do the right thing no matter how hard they try. So this request is, in my opinion, low priority at best.

Whereas here it's quite simple: Just adopt a convention for marking objects as unmoveable (for example, say _unmoveable = true in the entity def, similar to _mcl_pistons_unmovable = true).

Footnotes

  1. You can still use the methodtable hooking hack if you think this is the silver bullet. Doing the check in the engine wouldn't be free either, and the get_properties and set_properties calls associated could very well end up much more expensive.

@mruncreative
Copy link
Author

mruncreative commented Feb 3, 2025

Note also the "physical" object property which controls collisions. Graphical entities will have this set to false.

Neither Mesecons nor Redstone consider "physical" to be unmovable right now.

What about my target mod? It uses the object for the collision and selection box while the node is not walkable but pointable which allows it to get shot by raytracing weapons (which ignore non walkable nodes) and collision based weapons.

I'm not sure but the item frame might use an entity that is pointable.

What if you want a ghostlike creature that can get shot? Okay, why would you want a ghost to be affected by worldly effects? I'll think about the physical approach.

@sfan5 sfan5 changed the title Feature Request: unmovable initial property for entities unmovable property for entities Feb 3, 2025
@mruncreative
Copy link
Author

How about additional soft functions instead that can fail? Like soft_set_pos soft_set_velocity soft_set_accuracy.
Mods that apply stuff to unknown entities can use those insteads. Or instead of "failing" you could make them overridable.

@appgurueu
Copy link
Contributor

You're right that physical only allows making an educated guess, and as you show, it comes down to game design decisions.

soft_* methods don't feel like a clean solution to me (they feel like an "overly specific" abstraction: the user should just write out the "if movable then ... end" instead), but if you think they are, you can establish them in a mod or game. (Ideally namespaced such that they don't conflict with the engine namespace.)

@mruncreative
Copy link
Author

https://codeberg.org/mineclonia/mineclonia/issues/2780#issuecomment-2682091

_mcl_pistons_unmovable seems to have some sort of weird bugs. Unmovability of objects isn't as simple as it first seems.

@mruncreative
Copy link
Author

I already wrote a gamecompat function for mcla that shares unmov info between mesecons and redstone. However _mcl_pistons_unmovable doesn't even work right as of now. I can see it affecting the behavior in some cases though.

Everything under the sun requiring mesecons_mvps and mesecons_gamecompat seems a little overkill in my opinion.

@mruncreative
Copy link
Author

mruncreative commented Feb 4, 2025

And Cora would like a callback for entities being moved in Mineclonia. That could be implemented mod and game neutral as the "override functions" I mentioned which could then call the real set_pos or move_to if they want to.

It probably doesn't really matter if a piston is trying to move the object or anything else if the object wants to know about it.

@corarona
Copy link
Contributor

corarona commented Feb 5, 2025

And Cora would like a callback for entities being moved in Mineclonia.

I think you may have misunderstood that, I meant to say that I was going to implement such a callback in mcl_pistons. (EDIT: or rather that I previously already did but the PR got lost in WIP hell, i have revived those commits now: https://codeberg.org/mineclonia/mineclonia/pulls/2788 )

Such a callback from the engine may be nice however I am not exactly sure how such a thing might or should look so that it's actually useful.

An idea I had that is admittedly only tangential to this is to have the engine provide a way to easily manage these "node-bound" entities. I have seen this construct of "create entity when node is constructed, remove when destructed" etc. many times and we have it in mcl a bunch of times as well. Might be worthwhile to do that since it is always somewhat awkward to get this right, in particular you have to kind "fish" for the entity when you need to access it using get_objects_inside_radius or such.

@corarona
Copy link
Contributor

corarona commented Feb 5, 2025

_mcl_pistons_unmovable seems to have some sort of weird bugs. Unmovability of objects isn't as simple as it first seems.

it's a bug it should be fixed in the above mentioned PR

@appgurueu
Copy link
Contributor

appgurueu commented Feb 5, 2025

An idea I had that is admittedly only tangential to this is to have the engine provide a way to easily manage these "node-bound" entities.

#6963

you have to kind "fish" for the entity

Can't you make the entity "register" itself (e.g. in a table mapping node positions to entities) inside on_activate?

@mruncreative
Copy link
Author

mruncreative commented Feb 5, 2025

I did what appgurueu said to check for blood splatter to remove when nodes are dug with a simple table index.
You can use core.hash_node_position(pos) as index and index it in a table on_activate and remove it from there on_deactivate.
However this creates problems when the entity is moved which, as we know, can happen right now for all kinds of reasons. So what I did was store the position it has at activation in a variable on the entity and use the variable to remove it.

If we solve unmovability globally it would become easier.

@corarona
Copy link
Contributor

corarona commented Feb 6, 2025

Can't you make the entity "register" itself (e.g. in a table mapping node positions to entities) inside on_activate?

that's about how i did it in some places too yeah, i just thought since this is a concept used a lot and certainly not one without pitfalls either it might make sense for the engine to provide such a method... there's lots of things the engine provides that are possible to do completely in lua .. including unmovability btw :P

So what I did was store the position it has at activation in a variable on the entity and use the variable to remove it.

yeah i think i have in some cases regularly checked in on_step if the node is still there and remove the entity if not or something

@mruncreative
Copy link
Author

mruncreative commented Feb 9, 2025

A mod that transplants a region of the world to somewhere else. Suddenly some entities just arbitrarily get left behind. Makes no sense.

So I tested now what happens if I move an area of nodes with worldedit and what happens is that neither constructors nor destructors get called, the entities aren't moved and I end up with a loose entity and a node with no entity.

Should worldedit call constructors and destructors? My reasoning was that it could call the destructors (which destroy the entity), save it or move it and call the constructors again where it's placed and a new entity would be in the right spot. I assume that the serializable data is on the node meta anyway and not in the staticsave data of the entity.

Feel free to suggest alternative solutions to what I proposed for keeping nodes and entities together.

@sfan5
Copy link
Collaborator

sfan5 commented Feb 9, 2025

Should worldedit call constructors and destructors? My reasoning was that it could call the destructors (which destroy the entity), save it or move it and call the constructors again where it's placed and a new entity would be in the right spot.

WorldEdit intentionally uses vmanips, which do neither of those. Calling callbacks would be too expensive.

You could potentially argue that entities should also be moved, but this idea falls apart for set, replace and some other operations.

@mruncreative
Copy link
Author

Then what do we do about "node entities"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Issues that request the addition or enhancement of a feature @ Script API
Projects
None yet
Development

No branches or pull requests

5 participants