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

Add API function minetest.activate_objects_in_area #11630

Closed
wants to merge 8 commits into from

Conversation

TurkeyMcMac
Copy link
Contributor

@TurkeyMcMac TurkeyMcMac commented Sep 15, 2021

Add compact, short information about your PR for easier understanding:

  • Goal of the PR
    • The goal is to add a way to synchronously find the objects in any area.
  • How does the PR work?
    • The API function finds the mapblocks containing the given area. Then it goes through each of them and activates the objects using a new method of ServerEnvironment. The objects are activated like during regular mapblock activation, except that only those within the given area are activated.
    • The function cannot be called when activation or deactivation is going on. I don't think this restriction is very significant, although I could be wrong. Basically it just means you can't call the function in on_activate or on_deactivate. I had to change several places in the code to get this to work. It wouldn't be safe to let mods call the function in these places, since the activation and deactivation procedures are not written to be re-entrant.
  • Does it resolve any reported issue?
  • Does this relate to a goal in the roadmap?
    • It broadens the interface for objects/entities.
    • It counts as taking feedback from a modder.
  • If not a bug fix, why is this PR needed? What usecases does it solve?
    • I mentioned my usecase in the issue and the previous PR. It's for my mod area_containers.
    • @ElCeejo mentioned that my previous PR would be very useful to them in their work with objects, and this PR does basically the same thing in that regard.

To do

This PR is Ready for Review.

How to test

  1. Load a devtest world with this mod installed: test_activate_objects.tar.gz
  2. Run the command /grantme all.
  3. Start flying.
  4. Execute these commands:
    a. /teleport 0 6001 0
    b. /make_platform (0,6000,0)
    c. /teleport 0 10 0
  5. After the objects are deactivated, run /check_platform (0,6000,0). It should tell you that it activated the area and detected two objects.
  6. If you want to test that minetest.activate_objects_in_area throws an error in the right places, you can uncomment one or both of the latter two calls to the function in the mod's init.lua. These two calls are at the bottom of the file where the on_activate and on_deactivate methods of __builtin:item are overridden.

I've also now added a devtest command test_activate_objects for activating an arbitrary area.

@loffren174
Copy link

Does this allow to remove said objects then, as in, they could be listed with get_objects_in_radius and then removed one-by-one?
I assume so, but in #11599 sfan mentioned:

I don't see the point of having a list of static objects (that's what they're called) when there is no way to interact with them. For example you couldn't remove them.

An object equivalent to accompany the clearing of nodes between two positions would be very nice indeed for loading and unloading various schematic levels in the same worldspace without 'leftover' entities.

@TurkeyMcMac
Copy link
Contributor Author

@loffren174 I haven't tested the specific case of removing the objects, but it definitely should work. minetest.activate_objects_in_area would let you obtain the active object handles with which to remove the objects.

(I have tested manipulating the activated objects. For example, you can move them to an active mapblock to prevent them from being deactivated again.)

@sfan5 sfan5 added Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand @ Script API labels Oct 2, 2021
I don't fully understand all this reference stuff TBH.
@rubenwardy
Copy link
Member

Thanks for the contribution! 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.

If a core dev wants to support this PR, they may reopen it and apply the "Supported by core dev" label. In the future, you can open an issue first to get preapproval before working on something

@rubenwardy rubenwardy closed this Oct 31, 2021
@TurkeyMcMac
Copy link
Contributor Author

Could this be considered for reopening/inclusion in 5.6.0?

@SmallJoker
Copy link
Member

This PR was brought up in today's meeting.
https://irc.minetest.net/minetest-dev/2022-04-10#i_5959121

  1. What are the use-cases of this function? In which situation do you need to know entities of unloaded mapblocks?
  2. What are the downsides of Add an API function for synchronously activating areas #11609 ? That PR is more generic (allows more different operations), such as map changes.
  3. Is there a need for synchronous loading? A callback-based approach would be more efficient to not block the Lua side for too long.

@TurkeyMcMac
Copy link
Contributor Author

@SmallJoker

My specific use case is my mod area_containers. These containers are nodes which contain physical areas, in which one can put nodes and objects. I don't want people to be able to destroy containers which still have nodes or objects inside. Detecting nodes is easy, but detecting objects in a foolproof way is as yet impossible. My current mostly-working solution is to periodically store the count of objects inside whenever the mapblock representing the inside chamber is active. I could solve this cleanly using the function this PR adds.

Since the can_dig callback is synchronous, I need a synchronous function; the result must be available immediately.

The problem with #11609 was the potential reentrancy inherent to a synchronous API. There are many times during the activation of a mapblock when Lua callbacks can be called (e.g. ABMs, node timers.) Theoretically any of these callbacks could call the area loading function, with unpredictable results. The activation process was not written to be reentrant AFAICT. A safe solution is to use a "lock" to prevent reentrancy, as is done in this PR. However, if the function activated entire mapblocks, it would then be impossible to use it within node timer callbacks, object activation callbacks, ABMs, or LBMs. A function which only activates objects is only uncallable during object activation (and deactivation.) My main goal in the first place was the activation of objects; I don't see much point of doing the other stuff when you can already access nodes in inactive blocks.

@sfan5
Copy link
Member

sfan5 commented Apr 10, 2022

Someone brought up can_dig and I suspected that this was the reason you want it a sync API, the thing is I am very wary of adding any sync APIs because they block the entire server waiting for one particular thing (especially with emerging this is risky).

A function to activate a particular mapblock (not synchronous) sounds generally useful but in the end I think have to give up on solving your usecase directly. Maybe you'd be fine with some creative workarounds like only allowing your nodes to be dug after a short delay during which the relevant blocks are activated...

@TurkeyMcMac
Copy link
Contributor Author

Is there evidence that activating objects is especially expensive? Blocks may have to be loaded from the disk, but this is already a possibility with existing synchronous APIs such as minetest.load_area. Furthermore, just about any API can cause lag if a mod uses it poorly. I don't think APIs should be left out because of this.

The alternative to this PR is forceloading blocks. Besides being asynchronous, this isn't even guaranteed to work, since there is a limit on the number of blocks forceloaded. Given that objects are so important in Minetest, the impossibility of reliably detecting static objects seems like a significant issue. There should at least be a reliable asynchronous API, but I think a synchronous API would be highly preferable.

@sfan5
Copy link
Member

sfan5 commented Apr 14, 2022

Nothing about it is particularly expensive, I'm just wary of adding more sync apis.

Furthermore, just about any API can cause lag if a mod uses it poorly. I don't think APIs should be left out because of this.

Having only a synchronous API can be considered a problem too, then.

@TurkeyMcMac
Copy link
Contributor Author

TurkeyMcMac commented Apr 14, 2022

I could rename the function to minetest.activate_objects_in_area_sync then implement an async version something like this:

function core.activate_objects_in_area(pos1, pos2, callback)
  assert(type(pos1) == "table" and type(pos2) == "table" and type(callback) == "function", "Invalid invocation of minetest.activate_objects_in_area")
  pos1 = vector.copy(pos1)
  pos2 = vector.copy(pos2)
  core.after(0, function()
    core.activate_objects_in_area_sync(pos1, pos2)
    callback()
  end)
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detection of objects in inactive areas
5 participants