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

Implement zone priority #35782

Closed
wants to merge 2 commits into from
Closed

Conversation

jagoly
Copy link
Contributor

@jagoly jagoly commented Nov 30, 2019

Summary

SUMMARY: Interface "Adds zone priority"

Purpose of change

Change the way ordering of zones works. Each zone now has a "priority" value. This value will always be respected when choosing where loot should be sorted. This makes things easier for those who like to put a lot of time into sorting their stuff. This also allows re-ordering vehicle zones, which was previously not possible.

partly implements #34948

Describe the solution

The zone manager UI now displays priority next to each zone. Instead of just re-ordering the list, plus/minus will now increase/decrease this value. The list in the interface is sorted based on this value. The interface has been made slightly wider to accommodate showing priority.

I've also added a "priority" entry to every zone in loot_zones.json, which is just the default priority for a newly created zone. If players just choose to create a bunch of zones and don't change the order, then p.food will still override food, etc.

Finally, when loading old saves that don't have a priority in their zone data, the priority set in loot_zones.json will be applied, so current setups should continue to work as is.

Screenshot from 2019-11-30 15-23-19

Describe alternatives you've considered

Not being able to do these things.

Testing

Created lots of zones, sorted lots of items. Tested on my own saves which are quite zone-heavy. Loaded an old save to verify that legacy support works.

Additional context

This PR does not include my proposed removal of hardcoded loot zones, they are all still there.

Copy link
Member

@kevingranade kevingranade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so there are a number of issues here.
One, you made a bunch of only slightly related changes, and you only documented some of them in the PR description.
Two, we discussed this and I explicitly told you I don't want zone search to be exhaustive, and you made it exhaustive.

@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones labels Nov 30, 2019
@ghost
Copy link

ghost commented Dec 1, 2019

Have you tested how this affects NPC activities which make heavy use of the zones?

@jagoly
Copy link
Contributor Author

jagoly commented Dec 2, 2019

Have you tested how this affects NPC activities which make heavy use of the zones?

No, but it shouldn't affect them at all. The only thing that currently respects priority is ACT_SORT_LOOT, everything else should behave exactly as before. The priority could however be extended to other NPC activities, if one had a use for it.

One, you made a bunch of only slightly related changes, and you only documented some of them in the PR description.

My apologies! Could you elaborate on which changes you are referring to? If you are referring to the functions I removed from zone_manager, I did so because they were no longer used. Also I renamed the get_zones method (which I added a sort to), to get_all_zones, as that name seemed more appropriate. I also cleaned up some surrounding code in game::zone_manager which I of course had to change for priority.

Would you prefer that I update the PR to explicitly list every change, or that I only include the bare minimum code changes to support the feature? I did try to keep changes to a minimum, but I may have changed a few lines I didn't need to (like eaeeee9#diff-18513665750ef5adf42b5ec29e14162eR6288).

Two, we discussed this and I explicitly told you I don't want zone search to be exhaustive, and you made it exhaustive.

Ouch. I guess I misunderstood you then, or maybe I was only thinking about that in the context of the filter stuff. I had a version using a "cache" like before, where I did some performance tests, but (perhaps prematurely) concluded that it wasn't any faster, so went with the simpler solution.

Would you prefer that I go first check if an item is food, then get food zone points from area_cache, etc.? But then the problem is I need the priority of the zone, so would need to do get_zones(type,where,fac), which iterates through every zone anyway. Or I could change the cache to be map<tripoint,zone*> instead of point sets, but then that would affect other zone actions, and if you have multi-tile zones, would require testing them multiple times instead of just once.

Even then that still needs to be an exhaustive search, because I need to find the zone with highest priority, not just any matching zone, since that's the entire point of this PR. I suppose I could have another cache map that has zones sorted by priority, and then would just need to find the first one, but then what happens if that zone is full? I would think that any overflow should go into a matching zone with lower priority, rather than just be ignored, as is currently the case.

If I'm still misunderstanding what you mean, I'd greatly appreciate if you could explain in a bit more detail. Thanks for being patient with me :)

@kevingranade
Copy link
Member

Even then that still needs to be an exhaustive search, because I need to find the zone with highest priority, not just any matching zone,

That's exactly the problem, yes. You said you had a use case for prioritizing custom zones, which is fine because they require exhaustive enumeration anyway, but you didn't have a use case for prioritizing non-custom zones as they don't overlap anyway. But this goes ahead and does exhaustive search across non-custom zones anyway.

@jagoly
Copy link
Contributor Author

jagoly commented Dec 3, 2019

That's exactly the problem, yes. You said you had a use case for prioritizing custom zones, which is fine because they require exhaustive enumeration anyway, but you didn't have a use case for prioritizing non-custom zones as they don't overlap anyway. But this goes ahead and does exhaustive search across non-custom zones anyway.

As an example, right now I have a custom zone containing all my sealed jars of food. I also have a perishable food zone in a fridge. Because I can give the perishable food zone a higher priority than my jar zone, once a jar is opened, it will be automatically moved into the fridge.

@kevingranade
Copy link
Member

Ok so... no. I'm not interested in adding priority across all fields if it means we have to do exhaustive search over them. Zones are a piece of infrastructure we are likely to use increasingly intensively for npc camp features, and I am not willing to add a feature that backs us into a corner performance wise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Inventory / AIM / Zones Inventory, Advanced Inventory Management or Zones [JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants