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

Fix Nonrigid nested container can't store picked up items #62073

Conversation

EIIKaO
Copy link
Contributor

@EIIKaO EIIKaO commented Nov 4, 2022

Summary

Bugfixes "Nonrigid nested containers can't store picked up items"

Purpose of change

Describe the solution

In current codes, nested non-rigid containers did not contain picked up count_by_charges items.
Improved the method to collect all pockets, even nested non-rigid pockets, calculate capacity limits by the parent-child relationship, and then fill the items based on priority.

Describe alternatives you've considered

None

Testing

In-game testing done.

Repro method by the issue #62068

  • Spawn a nonrigid container like a plastic zipper bag, as well as a wearable container like a backpack.
  • Wear the backpack, put the plastic zipper bag in the backpack.
  • Set the priority on the bag to a high value
  • (Optionally) set an item whitelist for a specific item that fits in the bag.
  • Spawn soap and put it on the ground.
  • Pick up the soap.
  • [NEW] Confirm that the soap were put into the plastic zipper bag.

Additional context

Thanks to @akrieger, his report, which accurately pointed out this problem, struck a chord with my laziness.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Nov 4, 2022
@mqrause
Copy link
Contributor

mqrause commented Nov 4, 2022

Can't do a full review right now, but it looks like most if not all you're doing there could be done with item_location already.

@EIIKaO
Copy link
Contributor Author

EIIKaO commented Nov 4, 2022

@mqrause Thanks for advice.
This is actually code I wrote about two months ago.
Since then I learned a bit more about item_location too. ( I haven't PR'd it yet, but the following content, related to the feature that allows follower to read e-books: EIIKaO@8d3f274 )

I would like to go over the code again myself, but it would be even more helpful if you could indicate more specific points( or direction ) of improvement .

@mqrause
Copy link
Contributor

mqrause commented Nov 4, 2022

I'll have to look at it a bit closer this weekend, but as a quick and hopefully helpful pointer:
You can build the top level outfit locations with item_location parent_location( guy, pointer_to_worn_item ) and then go deeper level by level with item_location content_location( parent_location, pointer_to_content_item ). With these you can call item_location::parents_can_contain_recursive and item_location::max_charges_by_parent_recursive to check if/how much of something really fits inside a nested container.

@EIIKaO
Copy link
Contributor Author

EIIKaO commented Nov 4, 2022

@mqrause Thanks for useful information.

After my earlier post, I have checked myself to see if I could make use of item_location, and have confirmed that it may at least substitute it with a tuple of item_location without using unique_ptr.

Now I will seek to simplify it further with the new information you have given me.

Use item_location instead of unique structure and unique_ptr
@EIIKaO
Copy link
Contributor Author

EIIKaO commented Nov 4, 2022

Following mqrause's advice, I adopted a style that utilizes item_location.
Now uses item_location's function to allow check if/how much of something really fits inside a nested container.

Unfortunately, my poor knowledge and skills, have resulted in the generation of code that is not very readable.

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 4, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 4, 2022
@anothersimulacrum
Copy link
Member

anothersimulacrum commented Nov 4, 2022

Would you be able to add a test for this case, so we can prevent bugs like this?
You may also be able to improve on readability by using a struct instead of a tuple.

@EIIKaO
Copy link
Contributor Author

EIIKaO commented Nov 5, 2022

Would you be able to add a test for this case, so we can prevent bugs like this?

I am interested but it may be difficult for me to make it.
One reason is that I do not have an environment to build the tests.

You may also be able to improve on readability by using a struct instead of a tuple.

I will try this one quickly.
Thank you for your advice.

Recreate structure using item_location
Fix logical bug
Add const and reference modifiers
src/character_attire.cpp Outdated Show resolved Hide resolved
src/character_attire.cpp Outdated Show resolved Hide resolved
src/character_attire.cpp Outdated Show resolved Hide resolved
src/character_attire.cpp Outdated Show resolved Hide resolved
EIIKaO and others added 2 commits November 13, 2022 04:10
Changed from individual emplace_back to insert.
Suggested by mqrause

Co-authored-by: mqrause <[email protected]>
get_child_pocket_with_parent func migrates from outfit scope to Character scope
get_child_pocket_with_parent func has new filter and sort function arguments
src/character_attire.cpp Outdated Show resolved Hide resolved
src/character_attire.cpp Outdated Show resolved Hide resolved
src/character_attire.cpp Outdated Show resolved Hide resolved
Suggested by mqrause

Erase a redundant line break
src/character_attire.cpp Outdated Show resolved Hide resolved
Erase dupulicated checks

Co-authored-by: mqrause <[email protected]>
@EIIKaO EIIKaO marked this pull request as ready for review November 13, 2022 15:06
@dseguin dseguin merged commit 47873f3 into CleverRaven:master Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nonrigid nested containers can't store picked up items
4 participants