OpenNefia icon indicating copy to clipboard operation
OpenNefia copied to clipboard

Standardize the `InstancedMap.types` field

Open Ruin0x11 opened this issue 3 years ago • 1 comments

I have no clue how the logic defined by the map types system should be handled. The way we do it now (by using map:has_type("world_map")) works, and there's no shame in just admitting that and keeping it as is. But the issue nags me. What if we wanted only some of the behavior that one map type offers while preventing the rest, like the fact that items are always cleaned up when a town map gets refreshed?

I was thinking of using capabilities, but I'm not really sure. At first I thought we would have something like ITownMap, IGuildMap, IDungeonMap and so on that would act like town, guild, dungeon and the like that are in the types table at present. But that didn't seem like a good idea to me, because all the capabilities would be used for is identification: is this a town or guild? There's no state or extra logic they would encapsulate. That's what having string types already accomplishes. When you have a hammer, everything starts to look like a nail.

Then I thought that maybe we could combine all the types together into one big IMapType capability, and have all the logic shared between the map types put in there. Like, if you wanted to specify a new map type where the items get cleaned up on refresh, you'd implement a function.

function ITownMap:are_items_cleaned_on_refresh()
   return true
end

-- ...

function IPlayerOwnedMap:are_items_cleaned_on_refresh()
   return false
end

This seems like a better idea to me. ITownMap would implement IMapType, which would implement ICapability, Then you'd call map:get_capability(IMapType) to check the behavior.

But I'm still not sure about special behaviors specific to single map types, like the shelter. Those depend on a string comparison with the map's _archetype identifier. The problem is that unlike map types, you can only have one archetype. Maybe we should allow having multiple archetypes instead? (That would cause a lot of other problems, like: what happens when the map is first generated? You probably don't want to run multiple map initialization callbacks at once, since you only expect to receive one map.)

Beyond just that: Is it even useful to have an IShelter capability? Would someone ever want to just take all the existing shelter logic and apply it to their own custom map, without needing to duplicate the map_archetype and all its callbacks? I'm not sure, but assuming that is the case, it sounds like there would be way too much work to pull that off when all the logic depends on the hardcoded check for map._archetype == "elona.shelter" in all the event handlers. Such a check screams to me that it should be implemented in some other way, but I'm not sure how. Maybe people should be able to add their own map types, and upgrade the check from an archetype (which each map can only have one of) to a map type (which each map can have multiple of)? Or simply merge the map types system with the archetypes system, and allow having multiple archetypes? I don't know.

Ruin0x11 avatar Mar 13 '21 20:03 Ruin0x11

I'm also wondering how this paradigm of creating capabilities fits in with the design pattern of using event handlers for a lot of things. What is the reason we wouldn't have a base.calc_are_items_cloned_on_refresh event that could be overridden, and instead implement it as a capability? Why wouldn't we use capabilities for callbacks on data entries, because those callbacks aren't "data" in the sense of prototype fields to be duplicated?

I think the reason for not having the event is because the set of map types is well defined. You always expect some behavior if you give a map a type, and if you don't expect that behavior then you'd implement a new interface that overrides it.

Then why aren't map types just types in data? There's no state that needs to be kept for them. We could have a set amount of fields defining map behavior and upgrade the map types to be data entry IDs into said data type.

If the argument is extensibility, then I'm wondering how mods would override the behavior that an existing capability provides if its interface uses hardcoded functions. Then maybe IMapType would not have children like ITownMap and IPlayerOwnedMap, and instead keep all its fields as booleans, "in case" someone wanted to override one.

Then how do those fields get initialized? Through data? Then we would have both capabilities and data entries for this map types feature. Is it necessary to have both?

For that example, I think data would be necessary for having the stable prototypes, and capabilities would be used for modding. Because otherwise you'd have to edit an entry in data to modify the behavior of a map type, and that would be global. Or you could copy one of the map type prototypes into your mod and change the fields you want. But that sounds brittle to me for some reason, maybe because we shouldn't care about what ID the type has, but what its behavior is defined as. The whole purpose of adding capabilities is so we wouldn't necessarily have to hardcode map:has_type("world_map") against the singular world_map type everywhere, and instead define the logic of map types in terms of how the map should behave at an abstract level. (It's the same problem as using instanceof with a concrete class everywhere instead of defining the behavior in terms of an interface.)

Ruin0x11 avatar Mar 13 '21 20:03 Ruin0x11