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

Convert non-AMMO items to GENERIC #18836

Merged
merged 7 commits into from Oct 31, 2016

Conversation

Projects
None yet
3 participants
@mugling
Copy link
Contributor

commented Oct 15, 2016

  • Exploits #18828 to convert some items from AMMO to GENERIC
  • Adds brass material for casings
  • Allows specifying item:damage_states (minimum and maximum repair) via JSON

@mugling mugling force-pushed the mugling:unammo1 branch 2 times, most recently Oct 15, 2016

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2016

Ready

@mugling mugling force-pushed the mugling:unammo1 branch to c7c10f3 Oct 15, 2016

@Reclusive-reptile

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2016

Considering that casings would be littered about in large numbers, (ie. a burst from the mini-gun) does the change from ammo to generic result in any slow down when large stacks of casings are spawned?

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2016

The code already spawns the cartridges individually so no that won't be changed

@Reclusive-reptile

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2016

So a stack of 1000 casings wouldn't hurt performance? I recall rags and other generic items causing problems in large numbers, in the crafting UI for example.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2016

That's actually an important observation.
Except maybe not performance itself, but memory.
How does it work with memory?

Also volume limit thing vs item count limit. Does it exhaust the volume limit?

I'd like the following tests (not unit, can be just "manual"):

  • Spawn 200 casings, drop them as one action
  • Measure memory of DDA, spawn 1000 casings, measure memory again, drop 1000 casings, measure memory
  • Spawn and drop 5000 casings

The above is because I really don't trust our item dropping system. I tried dropping 4000 rags and had to kill the process.

5000 one is an overkill so it's fine for it not to work, but the 1000 one should perform at least tolerably.

@Coolthulhu Coolthulhu self-assigned this Oct 20, 2016

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

Doesn't work well.
For one, dropping each casing takes a whole turn, meaning that you will spend 5 minutes dropping a pocketful of casings.
Dropping a significant number of casings (200, which can actually spawn) takes very noticeable amount of time.

@Coolthulhu Coolthulhu removed their assignment Oct 20, 2016

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2016

The memory issue isn't going to bite here but it could be a significant obstacle elsewhere so needs further consideration

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2016

So given item stacking is inherently evil (just the confounding with charges) see the simple changes in a02b828 that allow some GENERIC items to become stackable. We therefore get the performance advantage of stacking whilst still avoiding JSON obfuscation.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2016

Of the open PR's that I have #18836 and #19020 are the most important so marked as priority

@mugling mugling added the (P2 - High) label Oct 29, 2016

@Coolthulhu Coolthulhu self-assigned this Oct 31, 2016

@Coolthulhu Coolthulhu merged commit f8b435e into CleverRaven:master Oct 31, 2016

@mugling mugling referenced this pull request Nov 21, 2016

Closed

JSON API changes #19376

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.