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

Bugfix - dissection drop rate #62433

Merged
merged 13 commits into from
Dec 6, 2022

Conversation

cathalpern
Copy link
Contributor

Summary

Bugfixes "Dissection drop rate"

Purpose of change

Fixes #61423

Describe the solution

Previously, the section to set roll value had a condition where the value was being set, except in cases where the action was dissection. So, the roll value was always 0 for dissection, causing items to never be dropped in some cases. Removing the check that the action is not dissection allows the roll value to get set.

Describe alternatives you've considered

I'm just doing what the smart people said to do

Testing

Debug spawned and killed 10 prototype cyborgs. Was able to successfully harvest a Power Storage CBM from all.

@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 labels Nov 27, 2022
@RenechCDDA
Copy link
Member

RenechCDDA commented Nov 27, 2022

Well, trying it out now that I got back.

This isn't going to work, because it guarantees the result. Here's a quick test change I made and the results:
image
(Note: Setting the item group to a count of say 100-1000 produces a seemingly completely random range within those values, regardless of your degree of success or, y'know, roll, so that's... not good either. )
image
(Reported success rate includes corrected debug output)
Notice we should fail every single one of these dissections but we always get a power storage CBM. Essentially this just breaks it in the opposite direction by guaranteeing we get defined single items regardless of how badly we do on the dissection.

Some other things I'd also like to address to fully fix the issue:

-There's a typo in the debug display on line 362, 19.0f should change to 10.0f (like the line below it). The above images already include this fix, but it's important moving forward.

-Debug output doesn't appear on corpses which define a single item for their dissection drop (e.g. broken cyborg with cyborg_harvest removed).

@cathalpern cathalpern closed this Nov 27, 2022
@cathalpern
Copy link
Contributor Author

I did think that it was odd that I was successfully harvesting with things like 95% failure chances - but thought maybe that was intentional, in that certain things would always drop based on config.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 27, 2022
@I-am-Erk
Copy link
Member

I-am-Erk commented Dec 1, 2022

So, this does appear to be the correct change after some play testing in the discord. What we need is to go here:

"flags": [ "NO_STERILE", "NO_PACKED", "FILTHY" ],

and add something like

"base_num": [ 0, 3 ],
"scale_num": [ 0.5, 2.5 ]

to each entry in zomborg and broken_cyborg in that file. if someone could add that to this PR or a clone of this PR and test it, I'd be much obliged... preliminary reports are that this should work.

@cathalpern
Copy link
Contributor Author

Adding scale numbers to dissection JSON and reopening

@cathalpern cathalpern reopened this Dec 2, 2022
@github-actions github-actions bot added the [JSON] Changes (can be) made in JSON label Dec 2, 2022
data/json/harvest_dissect.json Outdated Show resolved Hide resolved
data/json/harvest_dissect.json Outdated Show resolved Hide resolved
data/json/harvest_dissect.json Outdated Show resolved Hide resolved
@github-actions github-actions bot removed json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Dec 2, 2022
cathalpern and others added 3 commits December 1, 2022 19:39
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot added the json-styled JSON lint passed, label assigned by github actions label Dec 2, 2022
@I-am-Erk
Copy link
Member

I-am-Erk commented Dec 2, 2022

I will try to review the JSON end tonight or tomorrow

@RenechCDDA
Copy link
Member

RenechCDDA commented Dec 2, 2022

Please include the typo fix mentioned earlier, in case this ends up being the preferred solution.

-There's a typo in the debug display on line 362, 19.0f should change to 10.0f (like the line below it). The above images already include this fix, but it's important moving forward.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 2, 2022
@mqrause
Copy link
Contributor

mqrause commented Dec 2, 2022

A nice to have would be a test that makes sure result numbers are within expectations. Doesn't necessarily have to be part of this PR, though.

@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Dec 2, 2022
src/activity_handlers.cpp Outdated Show resolved Hide resolved
src/activity_handlers.cpp Outdated Show resolved Hide resolved
src/activity_handlers.cpp Outdated Show resolved Hide resolved
src/activity_handlers.cpp Outdated Show resolved Hide resolved
cathalpern and others added 6 commits December 2, 2022 18:27
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/activity_handlers.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Dec 2, 2022
@mqrause
Copy link
Contributor

mqrause commented Dec 2, 2022

There's nothing wrong with leaving the debug messages in.

@cathalpern
Copy link
Contributor Author

cathalpern commented Dec 3, 2022

There's nothing wrong with leaving the debug messages in.

Eh, they were mostly just for checking the numbers as I was testing - don't want to spam the logs with stuff that wouldn't be super meaningful. If other people think the min/max and roll values would be helpful, I can make the messages a little more explicit and add them back. Also instinctive reaction to what felt like leaving a bunch of console.logs in my finalized code haha

@mqrause
Copy link
Contributor

mqrause commented Dec 3, 2022

There's a filter for debug messages and I wouldn't consider it spam in the first place. But of course there's also nothing wrong with removing them. And making them more compact and more meaningful is obviously also a good thing.

@Night-Pryanik Night-Pryanik added the (P2 - High) High priority (for ex. important bugfixes) label Dec 3, 2022
@dseguin dseguin merged commit 41cc871 into CleverRaven:master Dec 6, 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] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions (P2 - High) High priority (for ex. important bugfixes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prototype cyborgs and power storage cbm scarcity
6 participants