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

Allow Monkey used injector to generate CRISPR Charge #65552

Merged
merged 5 commits into from Apr 14, 2022
Merged

Allow Monkey used injector to generate CRISPR Charge #65552

merged 5 commits into from Apr 14, 2022

Conversation

MrNihil
Copy link
Contributor

@MrNihil MrNihil commented Mar 18, 2022

About The Pull Request

Changed DNA Console behavior so it will accept unfilled dna injector, allowing use of monkeys to generate a CRISPR charge, as per Wiki Genetics Guide statement that this should be possible.

Why It's Good For The Game

Fixes CRISPR charge to not be added despite the message saying so.

Changelog

🆑MrNihil
fix: fixed unfilled injector from monkey not generating CRISPR charge.

…illed injector, as per Wiki stating that monkeys can generate CRISPR charge too.
@tgstation-server tgstation-server added the Fix Rewrites a bug so it appears in different circumstances label Mar 18, 2022
@github-actions
Copy link
Contributor

This PR has been inactive for long enough to be automatically marked as stale. This means it is at risk of being auto closed in ~ 7 days, please address any outstanding review items and ensure your PR is finished, if these are all true and you are auto-staled anyway, you need to actively ask maintainers if your PR will be merged. Once you have done any of the previous actions then you should request a maintainer remove the stale label on your PR, to reset the stale timer. If you feel no maintainer will respond in that time, you may wish to close this PR youself, while you seek maintainer comment, as you will then be able to reopen the PR yourself

@github-actions github-actions bot added the Stale Even the uncaring universe rejects you, why even go on label Mar 26, 2022
Comment on lines 199 to 200
if(A.research && A.crispr_charge)
crispr_charges++
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reading through the code surrounding this, and I noticed that it's rather deeply indented (tabbed to the right)
image
It'd be pretty cool if you could make if(used) if(!used), and then return ..(), to make things a bit easier to read,
See https://github.com/tgstation/tgstation/blob/master/.github/guides/STYLE.md#use-early-returns

S not a you problem, but it'd be nice to see the code made less crummy.

In a perfect world we'd also do something like if(!A.research), but that gets mildly messy, so maybe not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ill look into guidelines and try to figure something out. What I saw is adding another nested if statement vs checking A.research twice and went with the first option for code-readability, since this is my first try with DM (but its kinda close logic-wise to what I already know).

Regarding A.research I will try to look into what it means first, as I assumed it was always true (since there is no code explaining it needed actual research and it works without the R&D node, or so I think).

Thank you for the link and review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course. If you need any help feel free to bug me yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Right now I am deep in RL now and wont be available for at least one week sadly. But in the end I want to do something about this code :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LemonInTheDark I cleaned this method a little bit. I noticed it mentions recycling injectors in the comments, but that functionality is lacking. Let me know if I understood your advice correctly.

Additionally I dont know much about this mysterious A.research, as I couldn't find where it is set to true (and obviously it has to be set somewhere, otherwise activators wouldn't work at all). My guess is that this var is useless and could be deleted, but I need some more time to figure it out.

I think It will be better to push this bug-fix first and concentrate on proper behavior later, as it doesn't seem to be a major issue for now. I plan to add some quality improvements later and could mix it with some fixes, like Genetic scanner showing which mutations are active, showing chromosome number even when researched for more clarity, or proposing some improvements like UE+UI scanning, blood DNA scan (it is a genetic scanner, right?) and syncing DNA research with R&D server, so discovered mutations could be shared among synced DNA Consoles (no more disk jogging between geneticists).

@LemonInTheDark LemonInTheDark removed the Stale Even the uncaring universe rejects you, why even go on label Mar 30, 2022
Copy link
Member

@LemonInTheDark LemonInTheDark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, Capitalized vars aren't standard, but almost anything's better then single letter ones.

code/game/machinery/computer/dna_console.dm Outdated Show resolved Hide resolved
code/game/machinery/computer/dna_console.dm Outdated Show resolved Hide resolved
MrNihil and others added 2 commits April 10, 2022 13:41
Co-authored-by: LemonInTheDark <58055496+LemonInTheDark@users.noreply.github.com>
Item and Activator from lowercase.
Co-authored-by: LemonInTheDark <58055496+LemonInTheDark@users.noreply.github.com>
@LemonInTheDark LemonInTheDark merged commit d041345 into tgstation:master Apr 14, 2022
github-actions bot added a commit that referenced this pull request Apr 14, 2022
LemonInTheDark pushed a commit to LemonInTheDark/tgstation that referenced this pull request Mar 12, 2023
…ion#399)

* Allow Monkey used injector to generate CRISPR Charge (tgstation#65552)

Changed DNA Console behavior so it will accept unfilled dna injector, allowing use of monkeys to generate a CRISPR charge, as per Wiki Genetics Guide statement that this should be possible.

Fixes CRISPR charge to not be added despite the message saying so.

* Allow Monkey used injector to generate CRISPR Charge

Co-authored-by: MrNihil <33802925+MrNihil@users.noreply.github.com>
Co-authored-by: Debian <debian@packer-output-01d6f1be-59bf-4994-80ec-c61b12fe30e1>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Rewrites a bug so it appears in different circumstances
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants