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

Reverse pickpocketing #1491

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@akortunov
Copy link
Contributor

akortunov commented Oct 3, 2017

This PR is aimed to implement feature #1336.

I have a lot of questions:

  1. Can this feature be implemented before 1.0?
  2. Should a victim use autoequipping if a player places clothing/armor to the victim's inventory?
  3. Can we merge this feature with pickpocketing mechanics fix and use it as "advanced pickpocketing" config option?
  4. Should we decrement a count of stolen items, if a player previously stole "reversed" item from the same victim?
  5. Should the player be able to "overburden" victim by reverse pickpocketing?

A relevant forum discussion is here.

@akortunov akortunov force-pushed the akortunov:reversePickpocketing branch from cfb4889 to d8ce806 Oct 4, 2017

@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Oct 4, 2017

I decided to move some logic from container.cpp to item models.

@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Oct 5, 2017

I think this PR is ready for testing and review.

@akortunov akortunov changed the title [Discussion] Reverse pickpocketing Reverse pickpocketing Oct 5, 2017

:Default: False

If this setting is true, an engine will use an item weight instead of value to calculate chance of successful pickpocketing.
Also a "reverse pickpocketing" feature will be enabled in this case.

This comment has been minimized.

@Capostrophic

Capostrophic Oct 5, 2017

Contributor

"the engine", "the item weight"/"the weight of an item", "the value", "the chance" and "the <...> feature" on this and previous lines. Definite article.

@akortunov akortunov force-pushed the akortunov:reversePickpocketing branch from 692888c to 55afd0c Oct 5, 2017

@drummyfish

This comment has been minimized.

Copy link
Contributor

drummyfish commented Oct 17, 2017

I think Skyrim/Oblivion had reverse pickpocketing, didn't they? We could look at how it worked there regarding the overburdening etc. I'll try to research this a bit.

EDIT:

  • In Oblivion it only works for zero-weight items.
  • In Skyrim:
    • It won't let you overcumber an NPC. Also I've read the heavier the item is, the less chance to successfully reverse pickpocket it has.
    • From forums I've understood that the NPCs will use a weapon you pickpocket on them if they find it better than what they have, but they won't equip armor you give them, with an exception of followers.
    • There's a perk that lets you pickpocket poisoned food on NPCs which they will then eat and die.
@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Oct 17, 2017

Well, then this PR should work as Skyrim without perk for now. The poisoning perk would require to implement poisons first.

@lysol90

This comment has been minimized.

Copy link
Contributor

lysol90 commented Nov 6, 2017

Maybe we should get @zinnschlag to comment on this..? It's been a while now since this PR was done.

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Nov 7, 2017

I commented on this issue on the forum already. But anyway:

This change seems to be outside of the scope of the OpenMW. It should be up to mod developers to make changes of this kind. I expect OpenMW to gain the necessary modding capabilities relatively quickly once we enter the post-1.0 de-hardcoding development phase. Hopefully 1.0 isn't too far off anymore (we made some substantial progress in the last couple of months). Now if 1.0 were still many years away I might consider this PR as a stopgap solution. But our current situation I just don't see the point.

@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Nov 7, 2017

Did anyone review/test this PR?

Amount of changes were made:

  1. Move model-specific logic from container.cpp to item models (no more "if dynamic_cast") - about 80% of changes
    We definely can use these changes before 1.0
  2. Reverse pickpocketing itself, just provides an option and overrides one method - about 15% of changes. Requires 1.
    It's hard to tell when the project will reach the end of post-1.0 de-hardcoding development phase.
    So maybe we will have to use some "stopgap solutions" for a couple of years.
  3. Change in success change formula - about 5% of changes
    I can revert it, if you do not like it. We indeed can provide a way to tweak the formula after 1.0
@ghost

This comment has been minimized.

Copy link

ghost commented Nov 7, 2017

My 2 cents:

  • Reverse pickpocket feature vs. the changed success formula are kinda unrelated - inevitably someone would ask to use one but not the other. Would not put them in the same option.
  • I don't see much of a use case for the reverse pickpocket feature at the moment, unless someone created a quest to depend on it; in which case, the setting would rather belong to the content file and/or flag specific NPCs as being able to be reverse-pickpocketed.
  • I don't see the current system as broken, it's just intentionally hard. Obviously, if I'm carrying very valuable things, I'm going to pay more attention to my surroundings, so using the item value kinda makes sense. If you think pickpocketing is too hard in general, you can mod 'fPickPocketChance' if you want.
  • I agree with Zini that this is at best a stopgap solution until these things are moved to scripts.
@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Nov 7, 2017

Well, can we use item models refactoring at least?

@zinnschlag

This comment has been minimized.

Copy link
Contributor

zinnschlag commented Nov 9, 2017

Not opposed to the idea. @scrawl ?

Of course that would require stripping out the game mechanics changes first. General tips: Separate changes belong into separate PRs. Mixing up a feature change and a refactoring pass in the same PR definitely has hurt the review attention you were getting.

@akortunov

This comment has been minimized.

Copy link
Contributor Author

akortunov commented Nov 12, 2017

Feel free to close this PR. If someone needs the alternative pickpocketing mechanics, he can take it from this branch.

@zinnschlag zinnschlag closed this Nov 12, 2017

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.