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

[CR] Implement item type slots for items to be of multiple types. #10402

Merged
merged 14 commits into from Dec 11, 2014

Conversation

Projects
None yet
4 participants
@BevapDin
Copy link
Contributor

commented Dec 9, 2014

Item slots replace (partially) the specific item subtypes: instead of an item type being it_armor, it is now of the base itype plus an armor slot. The armor slot contains the properties that have previously been stored in the it_armor.

This allows all items types to have armor properties, as all item types can have an armor slot. Access to the armor slot is wrapped in the item class (as was access to members of it_armor).

The same is done for containers.

I added the armor slot to all bows in archery.json, so they can be worn without modifications, but I'm not a bow person, so this is very much open for discussion. I also added the armor slot the wearable containers.

See #4189


TODO: document the stuff

Some things to consider:

  • I'd like to move the flags SEALS, WATERTIGHT, PRESERVES, RIGID into boolean properties of the container slot. They are of no use and no meaning outside of it (on non-container items).
  • Similar for armor flags (especially the layer).
  • Further separation into different slots: it_spoils (contains spoilage, so that non-food can spoil as well, may even contain a item type. hat it spoils into). it_spawns_in for the container the item spawns in (applies to ammo and food, but could be used for anything else as well).
  • Splitting bionic difficulty into install difficulty (property of the CBM item), and removal difficulty (property of the installed bionic). This would allow different CBM items (with different size/rarity/spawn location) that also differ by the install difficulty but grant the very same bionic. For example a CBM item commonly found in doctors office with high install difficulty (intended to be installed by experienced doctors) and another item (same bionic) very rarely found (in outposts?) that is easier to install but might be heavy and bulky.
  • Several item types use item::charges for the very same thing (chapters for books, amount of loaded ammo for guns and tools, number of items for ammo and food). This limits the the compatibility: an item can not be a book and a tool (with charges).
Removed copying of the itype
Using a pointer to the ammo type is completely fine here.
@narc0tiq

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2014

This looks like the first step to item composition, likely a very good and useful thing. I don't really find the dynamic_cast<it_armor> very nice to look at, and it can be confusing as you end up with two names for the same memory, either of which might get deleted at any time.

I have one minor quibble: I'd drop the "_slot" from the name. They're armor properties, you may as well be accessing them as it->armor->whatnot.

P.S.: Fix yo' compile errors. ;)

@BevapDin BevapDin force-pushed the BevapDin:armor-slot branch Dec 9, 2014

BevapDin added some commits Dec 9, 2014

Rename container member of it_ammo and it_comest to default_container.
Fix a bug in the consistency checking in item_factory: check tool != null and tool exists.

Add checking for it_ammo container being valid.
Prepare the item constructor for items of multiple types:
Instead of assuming that is_* of the item type will only be true for exactly one function (either tool or gun or gunmod or ...) it checks all types.
This allows proper initialization for all types in one item.

Also do the same thing in item::info.

Remove redundant writing of charges=-1, this is already done by item::init.
And the check should be directly for count_by_charges, because that what's important here: starting with item::charges > 0 instead of the default -1.
Combine code for food item info:
food in a container and food outside of it uses the same code now.
Prevent removal of shoulder strap from worn gun.
Also remove an outdated debug message. It does not matter whether the item is armor, the item functions used there will work fine for non-armor.

@BevapDin BevapDin force-pushed the BevapDin:armor-slot branch Dec 9, 2014

BevapDin added some commits Dec 9, 2014

Avoid direct usage of it_container::contains
Use item::get_remaining_capacity_for_liquid instead.
Implemented the item slot for container items:
This slot stores container content size (not item size).
Add armor slot data to wearable containers.
Removes the IS_ARMOR tag because the presence of the armor slot data makes the item armor.

@BevapDin BevapDin force-pushed the BevapDin:armor-slot branch to ec63577 Dec 9, 2014

@BevapDin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2014

Compiler errors? What compiler errors. There are now compile errors. (fixed and rebased)

I also renamed the itype members as requested.

This looks like the first step to item composition

That's the goal.

@KA101

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2014

This ready to go?

@kevingranade

This comment has been minimized.

Copy link
Member

commented Dec 10, 2014

Awesome, just what I was hoping for.
I like all your ideas in your TODO.
Regarding charges, the only thing that tomes to mind is to stick those values in item_vars. i.e. gun "charges" becomes item_vars["ammo_count"], battery charges for tools becomes item_vars["battery_charge"]. That refactoring can happen as each itype that uses charges is migrated to using a slot.

BevapDin added some commits Dec 10, 2014

Move container flags into boolean members of the container slot.
Also moves them from being common flags into container specific members.
Update documentation, add lines for each container property into the item info data.
@BevapDin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2014

I added documentation and moved the container flags into boolean members. Those container flags are now shown in the item description, along with the content size of the container (this already included in some item descriptions but not in all and getting the content size from the item type directly makes sure it's always correct).

And I declare this ready.

@KA101 KA101 self-assigned this Dec 10, 2014

@KA101 KA101 merged commit 251f985 into CleverRaven:master Dec 11, 2014

1 check passed

default
Details

@BevapDin BevapDin deleted the BevapDin:armor-slot branch Dec 11, 2014

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.