ldtteam
No kids allowed
Closes None
Changes proposed in this pull request
- Create a minimal flag-based method in IBuilding that supports building-based logic in a lightweight manner.
- Use this to mark certain buildings as not appropriate for a kid to visit.
- Apply the flag to the Nether Miner building.
- Intention is to allow add-on mods to be able to leverage this without needing an IBuilding interface change each time (hence the flag-based approach).
Testing
- [X] Yes I tested this before submitting it.
- [ ] I also did a multiplayer test.
Review please
I don't get the logic behind the flag, because regardless of what it'll require a modification to your custom building code. So whether you're setting a flag or you're having to override a new method.
Personally I don't like this system because it opens up allowing to query any random string, there's no "known" list of flags a player can choose from.
I don't get the logic behind the flag, because regardless of what it'll require a modification to your custom building code. So whether you're setting a flag or you're having to override a new method.
Personally I don't like this system because it opens up allowing to query any random string, there's no "known" list of flags a player can choose from.
Agreed that it will require a code change for the custom building - but it will not require an interface change to IBuilding. So add-on mod developers can make their customizations using this method, and not need to add anything new to the interface itself. So in this example I could have added a "canKidsVisit" method or something and it would have supported this use case - but only this use case.
I understand the point about lack of clarity with regards to what flags are available and have logic associated with them. My opinion is that the flexibility is worth the lack of clarity, but happy to revise if the prevailing opinion is otherwise!
An Enum would work much better for that, than a String
My only quibble with this is that adding to the enum would still require changes to the MineColonies codebase (rather than allowing add-on mod changes without necessitating that change). I don't feel strongly about this, though...
An Enum would work much better for that, than a String
My only quibble with this is that adding to the enum would still require changes to the MineColonies codebase (rather than allowing add-on mod changes without necessitating that change). I don't feel strongly about this, though...
It doesn't matter if you need updates or not, under any case you're going to have to update Minecolonies if you actually want access to the new flags, so what's the point?
Any new flags, that only make sense for an addon, don’t need to be added to the minecolonies codebase in a new PR
So it is now quite easy to flag (literally) a few buildings as “trade_building”, if that is where an entity from a trade addon would need to go to, without first needing to PR an isTradeBuilding function, which is useless in minecolonies itself (until trading would be implemented, of course, I may not have chosen the best example)
It doesn't matter if you need updates or not, under any case you're going to have to update Minecolonies if you actually want access to the new flags, so what's the point?
Perhaps I'm misunderstanding the feedback, but I don't see it that way. I can write code in my add-on module that sets flags that I entirely control, and which my logic can use as needed, and I can rely on them being available on anything with the IBuilding interface - so I wouldn't need to update MineColonies to support any of that. Hypothetical example - if I have a set of buildings that count as "pet can work here", I can flag my own buildings and still interact with the buildings using IBuilding rather than introducing instanceof checks everywhere.
Similarly, as long as I'm handling the persistence in the add-on mod, I can flag buildings in the colony for use in WorkerAI logic. For example, I could flag any building connected to the train station with a "connected_by_track" flag, and have logic that uses it through IBuilding.
I agree that for a use case where the MineColonies native AI or buildings need to be aware of it (like this "kids won't visit" logic) that it would still require a MineColonies PR.
I mean if your sense is addons, then this is a no-go at all. We don't have to add a feature flag system to allow addons to define feature flags in, which are going to pollute into our code.
If you need custom logic on any of your own buildings, you add feature flags into your own abstract building class, not into ours.
All conditional things here should go via default/abstract base methods, not via such a feature flag system.
I mean if your sense is addons, then this is a no-go at all. We don't have to add a feature flag system to allow addons to define feature flags in, which are going to pollute into our code.
If you need custom logic on any of your own buildings, you add feature flags into your own abstract building class, not into ours.
All conditional things here should go via default/abstract base methods, not via such a feature flag system.
It is fine to add API for addons to interact with our systems. We already have the schematic tags though and modules that could allow for similar functionality
The point is that these are not generic kind of things, it's a getter. It's literally a method on the building canKidsVisitBuilding, that defaults to true. That's all this PR needs to be.
It is fine to add API for addons to interact with our systems. We already have the schematic tags though and modules that could allow for similar functionality
With this use case in mind, does the schematic tagging seem like a good solution? (I'm happy with it, just testing whether this is a "for example" suggestion, or one you're happy to have implemented for this scenario.)
I would get rid of hasFlag and the associated set, and create a helper in the EntityAICitizenChild class that uses IBuilding.getLocationsFromTag to determine based on schematic tags if the building is visitable by kids.
The point is that these are not generic kind of things, it's a getter. It's literally a method on the building canKidsVisitBuilding, that defaults to true. That's all this PR needs to be.
Part of why I didn't want to just add a getter is that every time building-based logic is tweaked like this the IBuilding interface will expand a bit. Not a big deal, but I have worked with teams that would rather have fewer PRs to evaluate, and push the independence out to the interface caller.
Schematic tagging would allow variation by style, which seems useful.
I appreciate the feedback, in any case, not trying to drive this point to death and happy to follow your preferences. :)
I would not do this with a tag, as it shouldn't be set on a per schematic basis. Modules are too cumbersome for a simple getter, you'd have to implement way too much. These kinds of simple checks are realistically something that the building instance configures.
Converted to a simple getter, and smartened up the logic to look at the parent (where applicable) as well.
For most of the use cases cited above it does sound like a custom module might be a better solution; for example rather than having a flag for "is connected to track network" or "has trade system" you could add a RailwayModule or TradeModule that can contain a whole bunch of things -- or just simple get/set/persist. The main caveat is that at present I don't think there's a nice API for externally adding additional modules to existing building types. In theory, the extra modules and their persisted data should already automatically vanish if you remove the addon mod as well.
Conversely, the flag system will get flags "stuck" if the addon is removed, but does make it easier for "sideways addons" where addon B could add a flag that only addon A uses, without having to directly depend on addon A. But being restricted only to flags does seem significantly limiting, vs. having a module.
Modules can also provide UI, though are not required to.
(Don't actually change anything yet, this is just musing out loud.)
