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

Vehicle zones #26921

Merged
merged 27 commits into from Dec 24, 2018

Conversation

Projects
None yet
5 participants
@Karthas077
Copy link
Contributor

commented Dec 3, 2018

Summary

SUMMARY: Content "Enable Loot Zones to bind to vehicle Cargo parts"

Purpose of change

Loot zones could not place items into, or take items from vehicle cargo. This enables that, as well as allowing the zone to move when the vehicle does.

Describe the solution

Vehicles keep track of zones which are created on their cargo parts (They must be 1x1).
The map keeps a list of all vehicles with zones, and is responsible for flagging their zones for position updates when they move.
The zone manager now keeps a separate cache for vehicle zones. It can query the map to ask if it is out of date, and receive updated zone data if it is.
The activity handler now checks for vehicle parts similar to the advanced inventory, and uses those parts as both source and destination as needed.

Describe alternatives you've considered

Linking vehicles and zones together and allow them to communicate directly introduces a number of problems including identifying when a vehicle is loaded into memory and available.

Additional context

Vehicle zones cannot currently be re-positioned and must be removed and re-added. This functionality can be added at a later time.

Loot zones which are not bound to a vehicle, but happen to have a vehicle placed in that location will still use the cargo as either source or destination. This is intended as a feature, as it allows for "Loading/Unloading Zones" where you can park ANY vehicle, instead of having to create an unloading zone for each vehicle you might park.

Karthas077 added some commits Nov 26, 2018

Vehicle Zones
Allowing vehicles to store zones and save them
Vehicle Zone Tracking
Moved vehicle tracking to map
Vehicle Loot Sorting
Taking items from and dropping items into vehicle loot zones
Vehicle zone source fix
Also Astyle

//Check source for cargo part
//map_stack and vehicle_stack are different types
//No way to avoid for loop duplication

This comment has been minimized.

Copy link
@BevapDin

BevapDin Dec 3, 2018

Contributor

Yes, there is: write a function template (with vehicle_stack / map_stack being the template arguments).

This comment has been minimized.

Copy link
@Karthas077

Karthas077 Dec 3, 2018

Author Contributor

I've never really learned about c++ template stuff properly. I can read them and sortof understand what they're doing but don't know how to write them from scratch to meet a given requirement.

This comment has been minimized.

Copy link
@BevapDin

BevapDin Dec 6, 2018

Contributor

I was mostly agitated by the comment itself (being incorrect). You can also use the runtime polymorphism: map_stackand vehicle_stack inherit from item_stack and the later provides all the functions you need here (to iterate over it).

This comment has been minimized.

Copy link
@Karthas077

Karthas077 Dec 6, 2018

Author Contributor

I tried declaring an item_stack type because I saw they both inherited from it, but it's a purely virtual class so it wouldn't let me :/

This comment has been minimized.

Copy link
@jbytheway

jbytheway Dec 6, 2018

Contributor

You'll need to make it a reference (or pointer) to an item_stack. You can't have an actual item_stack, but you can have a reference to a thing that derives from it.

This comment has been minimized.

Copy link
@Karthas077

Karthas077 Dec 7, 2018

Author Contributor

All the examples of abstract class polymorphism show how to declare it as a pointer, but doing so makes Gorgon angry, and I have no idea how to declare an item_stack reference without supplying an initializer to allow it to be either type.

This comment has been minimized.

Copy link
@jbytheway

jbytheway Dec 7, 2018

Contributor

You have to initialize a reference at declaration time. Which in this case is going to be a bit tricky. The neatest way I can think of would be to put the for loop in a lambda taking a const item_stack& and call that lambda in the two places.

This comment has been minimized.

Copy link
@Karthas077

Karthas077 Dec 7, 2018

Author Contributor

I don't think it can be a const item_stack because the vector it's being shoved into can't hold const items because they're being mutated, but it looks like the lambda worked. Thanks.

Edit: Once again it compiles for me but Gorgon doesn't like it.

Show resolved Hide resolved src/clzones.cpp Outdated
Show resolved Hide resolved src/map.cpp Outdated

@Karthas077 Karthas077 force-pushed the Karthas077:VehicleZones branch from d95d139 to 135b566 Dec 4, 2018

Show resolved Hide resolved src/activity_item_handling.cpp Outdated
Show resolved Hide resolved src/vehicle.cpp Outdated

Karthas077 added some commits Dec 4, 2018

Show resolved Hide resolved src/clzones.cpp Outdated
Show resolved Hide resolved src/clzones.cpp Outdated

BevapDin and others added some commits Dec 6, 2018

Update tripoint comparison
Co-Authored-By: Karthas077 <Karthas077@users.noreply.github.com>

@ZhilkinSerg ZhilkinSerg self-assigned this Dec 18, 2018

@ZhilkinSerg

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

Small issues found:

  • it is possible to add non-loot zones (e.g farm plot to vehicles);
  • vehicle loot zones aren't marked in the UI - some vehicle indicator would be nice to have.
@Karthas077

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

Small issues found:

  • it is possible to add non-loot zones (e.g farm plot to vehicles);
  • vehicle loot zones aren't marked in the UI - some vehicle indicator would be nice to have.

Second thing is easy, I'll get that done in a moment. The first one I honestly have no clue how to fix that isn't super ugly/hacky/likely to break something because the entire zone_options class and functionality makes zero sense to me.

Edit: And considering that the current implementation allows you to declare any zone on any tile without any sort of validation makes me believe that such handling is slightly beyond the scope of this PR

Edit2: Went ahead an implemented the first thing anyway.

@Karthas077

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Not a very fancy or clear indicator at the moment but there isn't a ton of space to work with.

Another "bug" that is more of a problem with the current implementation is that Vehicle Zones are not subject to the "Save/revert changes" functionality of the rest of the zones because their data is stored in the vehicle. Unfortunately there really isn't a way around that without very tightly coupling vehicles and zones which I was explicitly told not to do, or completely re-writing the way the reverting is handled to not rely on a .json file.

@Karthas077

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Added a Y/N query instead of automatically binding to the cargo part. Let me know if I need to do something to enable translating of that string.

Show resolved Hide resolved src/clzones.cpp Outdated

Karthas077 added some commits Dec 21, 2018

Implemented reverting vehicle zone changes
Also preventing adding Farm plots to vehicles and cleaned up cache iteration

@ZhilkinSerg ZhilkinSerg merged commit d2750f8 into CleverRaven:master Dec 24, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gorgon-ghprb Build finished.
Details

@ZhilkinSerg ZhilkinSerg removed their assignment Dec 24, 2018

@kevingranade

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/hybrid-inventory-2018-12-26/15953/76

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.