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

Allow long salvage with wielded and worn items #11562

Merged
merged 5 commits into from Mar 17, 2015

Conversation

Projects
None yet
4 participants
@Coolthulhu
Copy link
Contributor

commented Mar 13, 2015

As in title.

Also commented out re-setting the activity on long salvage finish. Didn't see any problems in the testing.
This results in player not being asked to confirm continuing salvaging every time a monster shows up or makes a sound, only the first time (still asks every time player is attacked/hurt).

I wanted to add long salvage with sheathed items, but it's far harder than it looks. For one, items in containers don't have inventory indices, which are required by activities.

@BevapDin

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2015

I wanted to add long salvage with sheathed items, but it's far harder than it looks. For one, items in containers don't have inventory indices, which are required by activities.

I'm wondering: do you need to store the tool that is used? Why not simply look it up in the inventory when it's needed (right before invoking its iuse function). It is automatically chosen anyway, whether this happens when the activity is started or when the activity is doing something shouldn't matter.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2015

Could be a bit slow with large inventories. I'm getting slowdowns when building crafting inventory with 100+ items already.
While it would happen at a moment where slowdown is acceptable (player isn't queried for anything), I'd rather keep the index if it is possible.

Coolthulhu added some commits Mar 13, 2015

@Coolthulhu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2015

OK, added support for sheathed (contained in general) knives.

A bit ugly (I'm casting away constness), but shouldn't cause much slowdown.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2015

No one responded for a while, so I'll ask:
Is it OK that I cast away the constntess here or should I make an overload for items_with that returns non-const pointers?

@DavidKeaton

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2015

Which line of code? iirc as long as the original object itself is not const, there should be no issue. However if it is, undefined behaviour.

@BevapDin

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2015

In my opinion it's fine, but go ahead and duplicate that function and add the const modifier to avoid this debate.

Actually no, don't do this. Instead use the value returned by get_item_position (which works fine with a const item!) and get a non-const reference to the very same item with player::i_at;

const item *x = ...
const int pos = u.get_item_position( x );
item &non_const = u.i_at( pos );
@Coolthulhu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2015

In #11569 I need to access items by pointers (items in containers have no inventory slots), so if I need to stop casting away constness, I need to go with the overload.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2015

OK, no casting away the constness

@DavidKeaton

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2015

still clacking away at nestable containers

@KA101 KA101 merged commit fc15afa into CleverRaven:master Mar 17, 2015

1 check passed

default
Details

@Coolthulhu Coolthulhu deleted the cataclysmbnteam:longsalvage-inventory branch Apr 10, 2015

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.