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

Fixing sizing issues with downsizing/upsizing items and refitting #28123

Conversation

@MetroidHunter
Copy link
Contributor

commented Feb 7, 2019

Summary

SUMMARY: Bugfixes "Fixing sizing issues with downsizing/upsizing when you are smaller/larger size"

Purpose of change

Fixes #27641

Describe the solution

  • Add ability to query size of item relative to size of character
  • Expand descriptions for items when character is various sizes and gear is various sizes, fits or doesnt fit, and is refittable or isnt
  • Expanding encumberance penalties for small gear, normal character and normal gear/small character to encompass large gear and large characters and the combinations of small gear and small characters
  • Ensuring items can only be downsized/upsized if they actually can be with VARSIZE and the current rules of UNDERSIZED and OVERSIZE

Describe alternatives you've considered

There are several alternatives here. The encumberance does not have to be adjusted to consider oversized clothes but then there is a disconnect between the information presented to the player and the actual encumberance differences. It makes sense that gear sized for someone that stands 2 feet tall is nearly impossible to wear for someone who is huge (I imagine 10-12ft)

There are some questions here with things like security glasses or knit scarves. They sort of fall into the category of items like panties/bras etc. but with this rework, they now have encumberance penalities for Huge and Small characters. This makes sense in realism terms as if your head has grown 3 sizes or shrunk 3 sizes, glasses built for a normal human would not fit well.

Additional context

Large Character with Various Gear

Large gear, fits

large_char_large_fits

Large gear, doesnt fit

large_char_large_gear

Leather Backpack

large_char_leather_backpack

Zero encumberance, same across all character sizes

large_char_no_encumberance

Normal sized gear, cannot be modified

large_char_normal_cannot_refit

Normal sized gear, doesnt fit

large_char_normal_gear

Undersized gear

large_char_small_gear

Normal Character with Various Gear

Leather backpack

normal_char_leather_backpack

Normal gear, fits

normal_char_normal_fits

Normal gear, poor fit

normal_char_normal_poor_fit

Normal gear, cannot be modified

normal_char_normal_static

Oversized gear

normal_char_oversized

Undersized gear

normal_char_small_clothes

Small Character with Various Gear

Leather Backpack

small_char_leather_backpack

Normal sized gear, can resize

small_char_normal_can_resize

Normal sized gear, cannot resize

small_char_normal_cant_resize

Small sized gear, fits

small_char_small_fits

Oversized gear

small_char_xl_gear

A zero encumberance item, same across all character sizes

small_char_zero_enc_item

Making sizing a concept for items. Expanding descriptions for various…
… sizes. Encumberance affected by small and large sizes. Ensuring items cannot be downsizable/upsizable unless possible

@MetroidHunter MetroidHunter changed the title [WIP]Fixing sizing issues with downsizing/upsizing items and refitting Fixing sizing issues with downsizing/upsizing items and refitting Feb 8, 2019

@int-ua

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

Will safety glasses be right size for non-normal player when crafted?

@MetroidHunter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

Will safety glasses be right size for non-normal player when crafted?

No they wont be but that is also existing behavior. At head right now, if you are small and craft safety glasses, you will get normal sized glasses and have an encumbrance penalty. However, at head right now, if you are huge and craft safety glasses, you will get normal sized glasses but not an encumbrance penalty.

The difference now is that, instead of the encumbrance penalty while small just happening without any information on it, you are being told that you have an encumbrance penalty and, to bring huge and small in line with each other, you now get an encumbrance penalty when huge and are also told about it

It may make sense, in a different PR more than likely, to decouple resizing clothes from VARSIZE since the intention there is fit/poor fit and not sizing and add a new flag, DOWNSIZABLE. Then, if present, any clothing that is downsizable would be able to go from normal to small and small to normal and if you are small and you craft gear that is DOWNSIZABLE, then it can automatically be made with UNDERSIZE as a flag on it. We could add UPSIZABLE but currently, following design of OVERSIZE, nothing is ever meant to be UPSIZABLE as defined as able to be made OVERSIZE or be made normal from OVERSIZE.

I would say this is beyond the scope of this PR unless the current fix is unacceptable.

@ZhilkinSerg ZhilkinSerg self-assigned this Feb 10, 2019

@MetroidHunter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

@ZhilkinSerg Have you had a chance to review and validate this PR? Is there any feedback on the approach or changes I can make? No rush of course and thank you for all your efforts

@ZhilkinSerg

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2019

Didn't test it yet.

@dissociativity

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

How would this affect a player with near sighted?
Can a mouse make some kind of goggles for it that are adjustable regardless of size due to the implied strap?

@MetroidHunter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

It doesnt change any existing constraint on a small character with near sightedness. You at the moment on head will have increased encumberance from the goggles you make or find and will not be told that you cannot downsize it nor that they are too big. This change will now explicitly tell you they are too big and cannot be downsized.

I considered making crafted items be downsized when you craft them or making more items downsizable as outside the scope of this PR but im happy to revisit. The goal was keep scope only to fixing the lack of messaging but I also understand if that now surfaces some issues we may want to address

Show resolved Hide resolved src/item.h

@ifreund ifreund removed the 0.D Freeze label Mar 8, 2019

@ZhilkinSerg ZhilkinSerg removed their assignment Mar 9, 2019

@kevingranade
Copy link
Member

left a comment

Something about this has changed effective encumbrance, causing some unit test failures.

0.011 s: unskilled_shooter_accuracy
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cata_test is a Catch v2.5.0 host application.
Run with -? for options
Randomness seeded to: 1549689492
-------------------------------------------------------------------------------
competent_shooter_accuracy
-------------------------------------------------------------------------------
ranged_balance.cpp:235
...............................................................................
ranged_balance.cpp:202: FAILED:
  REQUIRE( shooter.encumb( bp ) == encumbrance )
with expansion:
  10 == 5
with message:
  Body Part: torso
0.008 s: Wield time test
-------------------------------------------------------------------------------
Wield time test
  Wielding 1 aspirin from inventory while wearing boxing gloves
-------------------------------------------------------------------------------
wield_times_test.cpp:68
...............................................................................
wield_times_test.cpp:24: FAILED:
  CHECK( move_cost <= max_cost )
with expansion:
  240 <= 187
with message:
  Strength:8
0.000 s: Wielding 1 aspirin from inventory while wearing boxing gloves
0.008 s: Wield time test
-------------------------------------------------------------------------------
Wield time test
  Wielding combat knife from inventory while wearing boxing gloves
-------------------------------------------------------------------------------
wield_times_test.cpp:70
...............................................................................
wield_times_test.cpp:24: FAILED:
  CHECK( move_cost <= max_cost )
with expansion:
  265 (0x109) <= 220
with message:
  Strength:8

AFAICT these are not intentional changes, so there's a bug somewhere in here that needs fixing.

@kevingranade kevingranade added this to In progress in Tailoring Overhaul Mar 12, 2019

@ZhilkinSerg ZhilkinSerg self-assigned this Mar 20, 2019

@MetroidHunter

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Something about this has changed effective encumbrance, causing some unit test failures.

Will fix. I've been unavailable recently but should be ramping back up into development soon

@ZhilkinSerg ZhilkinSerg removed their assignment Mar 26, 2019

Merge branch 'master' of github.com:MetroidHunter/Cataclysm-DDA into …
…feature/CDDA-27641_resizing_items_and_sizing_improvements
@MetroidHunter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Something about this has changed effective encumbrance, causing some unit test failures.

The ranged test creates a shooter with a cloak_wool which has 5 encumbrance but is marked OVERSIZE. From my changes in item::get_encumber_when_containing, when we calculate encumbrance per item, we double encumberance for oversized items on normal characters now and so the output of:

ranged_balance.cpp:202: FAILED:
REQUIRE( shooter.encumb( bp ) == encumbrance )
with expansion:
10 == 5
with message:
Body Part: torso

is that doubling shown from 10 to 5. This is working as intended by this PR

The wield time tests all involve the boxing_glove which is also an item that is marked OVERSIZE and so is beholden to the same rules. Instead of an expected encumbrance of 70, we get 140 which gets added to the base cost of handling something (100) to get our 240 moves instead of the expected 170. This is also working as intended for the PR.

wield_times_test.cpp:24: FAILED:
CHECK( move_cost <= max_cost )
with expansion:
240 <= 187

The other tests are the same dealing with the boxing_gloves

AFAICT these are not intentional changes, so there's a bug somewhere in here that needs fixing.

I see 3 solutions:

  1. Remove the penalties for OVERSIZE added here. I think they add some degree of realism to the game and would not particularly want to do that but I do agree that the definitions are loose atm so that might be best.
  2. Remove OVERSIZE from the boxing_gloves and wool_coat and also the wedding_veil, which would be to say that these items are not OVERSIZE but the definition of the flag in the system
  3. Update the tests to reflect the new OVERSIZE penalty. This would be to say that the items marked OVERSIZE are correct and the boxing_gloves, wool_coat, et al are correct to have this penalty. Optionally, perhaps I should also audit what is marked OVERSIZE and make a note of what OVERSIZE means in this new context so that further items are marked accordingly

What are your thoughts @kevingranade ?

Merge branch 'master' of github.com:MetroidHunter/Cataclysm-DDA into …
…feature/CDDA-27641_resizing_items_and_sizing_improvements
@kevingranade

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

Sorry I didn't get back to this sooner, I lost track of it.
In short, remove the extra penalty for OVERSIZE. It wasn't called out in the PR description, I'm not clear on the rationale for it, and I expect items with OVERSIZE to already have an encumbrance penalty assessed if they need it, so the penalty doubles up on it.

Side note: Hooray, the test is working as intended by surfacing unexpected changes.

@ZhilkinSerg ZhilkinSerg self-assigned this Apr 21, 2019

@MetroidHunter

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Sorry I didn't get back to this sooner, I lost track of it.
In short, remove the extra penalty for OVERSIZE. It wasn't called out in the PR description, I'm not clear on the rationale for it, and I expect items with OVERSIZE to already have an encumbrance penalty assessed if they need it, so the penalty doubles up on it.

Side note: Hooray, the test is working as intended by surfacing unexpected changes.

Hooray indeed!

Also, my bad. The PR description says

Expanding encumberance penalties for small gear, normal character and normal gear/small character to encompass large gear and large characters and the combinations of small gear and small characters

But I can definitely see how that is not clear what is actually being changed. I will try to be clearer in the future.

I will make these changes now, fix conflicts, and roll out

@MetroidHunter

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Gorgon is having trouble accessing its origin remote. Is that expected?

@ZhilkinSerg

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Gorgon is having trouble accessing its origin remote. Is that expected?

That was some temporary issue probably.

@kevingranade kevingranade merged commit 70a8c03 into CleverRaven:master May 11, 2019

2 of 3 checks passed

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

Tailoring Overhaul automation moved this from In progress to Done May 11, 2019

@ZhilkinSerg ZhilkinSerg removed their assignment May 11, 2019

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.