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

Remove items that shatter instead of dropping them intact #48712

Merged

Conversation

Zeropol
Copy link
Contributor

@Zeropol Zeropol commented Apr 30, 2021

Summary

Bugfixes "items that shatter were not destroyed on impact. To fix #48707"

Purpose of change

Thrown glass item can display the message "the X shatters", but the item itself was still intact. Led to content duplication for say non empty bottles.
This pull request should remove the shattered item from the game, but not its content.

Describe the solution

Replaced the "drop_item.visit_items" then "add_item_or_charges" with "item( drop_item ).spill_contents;"
This way, the container should never be dropped.
I used this because it is what objects that should burst ( filled condoms ) use, and it works well for the condom.
I just copy pasted.

Describe alternatives you've considered

Also drop glass shards depending of the weight of the object.

Testing

Tests I did :

  • Threw empty bottles and saw they are removed from the game when the message say they shatter
  • Threw glass jars ( forgot if empty or not ) until one of them don't shatter, and saw it is not removed from the game.
  • Threw filled 3L jars ( salt water ) and saw they are removed and spill their content.
  • Threw filled 3L jar with multiples items ( game watch with its battery and glass shiver ) . Jar destroyed, game watch OK, glass shiv intact.
  • Threw empty and filled 3L jars ( salt water ) to a chicken until it die to be sure it still work.
  • used "make [..] RUNTESTS=1"
  • created a test item made of glass and able to both hold a battery and liquid. Threw the item. The item shatter and is removed, the liquid drop, the battery drop.

Tests that need to be done :

  • I have not identified others special cases but it would be relevant if someone comfortable with the code could verify this modification don't brake anything I did not though of ( maybe it was for a good reason that the shattered snippet did not use item.spill_contents )

Additional context

Sorry if I made someone loose its time testing a flawed PR. ( also I forgot to name and describe the commit )

@BrettDong BrettDong added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Items / Item Actions / Item Qualities Items and how they work and interact labels Apr 30, 2021
@Zeropol Zeropol changed the title Items that shatter were not destroyed on impact. Remove items that shatter instead of dropping them intact Apr 30, 2021
@actual-nh
Copy link
Contributor

Also drop glass shards depending of the weight of the object.

This should be done, but perhaps wait for this part until after the problem with slowdowns from lots of objects (#47007 and others) is (mostly?) fixed?

@actual-nh actual-nh added the Items: Containers Things that hold other things label Apr 30, 2021
@Zeropol
Copy link
Contributor Author

Zeropol commented Apr 30, 2021

If it is relevant I'll consider it then, thanks for the indication ! ( except if someone do it of course )

The slowdown caused by thousands of glass shards is maybe mostly caused by broken windows in cities, while the players ( I think ) usually don't throw molotovs/glass items every minute so it should not contribute that much to the glass shard abundance. ( while still be useful for "innawood" character who need a few shards ).

If I do it I propose to submit it in a different PR so if it can hurt performance it is easier to sort ( I suppose ).

@Zeropol
Copy link
Contributor Author

Zeropol commented May 1, 2021

In regard to make shattered item drop shards, I ran into the problem that the function that return the mass of an item is only partially implemented.

units::mass weight( bool include_contents = true, bool integral = false ) const;

The parameter include_contents is not used.

As a consequence, filled glass containers would drop more shards than the same container when empty.

I also noticed some values I question :

  • 3L jar have density of 93 kg/m³, its mass of 0.36 kg disassemble into 5 shards.
  • jar have density of 220 kg/m³, its mass of 0.15 kg disassemble into 2 shards.
  • bottle have density of 341 kg/m³, its mass of 0.45 kg disassemble into 3 shards.

As a result, shattered 3L jar would drop less shards than a bottle.
( instinctively, 3L jar could be heavier, or bottle could disassemble into more shards )

Would it make sense to determine the number of shards based on the volume instead of the mass ? I'm not seeing what side effect it could cause, if it do.
( looks like units::volume volume( bool integral = false, bool ignore_contents = false ) const; is fully implemented )

If this question is out of scope for this particular PR, please tell me so I ask for insights in the Discord instead of here.
Thanks :)

@Alex-Folts
Copy link
Contributor

In regard to make shattered item drop shards, I ran into the problem that the function that return the mass of an item is only partially implemented.

units::mass weight( bool include_contents = true, bool integral = false ) const;

The parameter include_contents is not used.

I guess you should use this method, but create new issue about this method is not fully implemented so it would be resolved as separate PR.

@Zeropol
Copy link
Contributor Author

Zeropol commented May 1, 2021

Thanks you for the insight Alex-Folts !

I also thought of using the itype instead of the actual item ( the mass should always the mass of the empty object with this method I hope ), I will try, but before I'll follow your advice and create an issue.
edit : I had it working :D and will submit the PR later today

@Alex-Folts
Copy link
Contributor

Also some time ago was related to glass shards PR: #42212
with this technically you can craft galss shards from glass objects, but smashing glass object against wall to make glass shards sounds more natural.

@Rivet-the-Zombie Rivet-the-Zombie merged commit 67105a9 into CleverRaven:master May 4, 2021
@Zeropol Zeropol deleted the shatter_item_dont_destroy branch May 4, 2021 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Items: Containers Things that hold other things Items / Item Actions / Item Qualities Items and how they work and interact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fragile items (e.g glass bottles) don't shatter when thrown
5 participants