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

Code fix for #71971 Water purification tablets only purify up to 4 water each #76326

Merged
merged 16 commits into from
Sep 12, 2024

Conversation

PatrikLundell
Copy link
Contributor

@PatrikLundell PatrikLundell commented Sep 10, 2024

Summary

None

Purpose of change

Fix compilation errors in #71971, requested by @Maleclypse for merge into that PR.

Describe the solution

  • Adapt to typification (conversion to tripoint_bub_ms from untyped tripoint).
  • Add missing constants in tests. No idea why they weren't there.
  • Change failed usage of protected field to use the corresponding operation in line with other usage.

Describe alternatives you've considered

Just do the tripoints.

Testing

Loaded a "random" save without any crashes or error messages.

Additional context

As stated above, this PR is intended to be used "sideways". As such, I didn't bother to give this PR a readable title, just a PR number reference, which I strongly discourage in general practice.

@github-actions github-actions bot added <Bugfix> This is a fix for a bug (or closes open issue) Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON Map / Mapgen Overmap, Mapgen, Map extras, Map display Items: Food / Vitamins Comestibles and drinks Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Spawn Creatures, items, vehicles, locations appearing on map Code: Tests Measurement, self-control, statistics, balancing. [C++] Changes (can be) made in C++. Previously named `Code` EOC: Effects On Condition Anything concerning Effects On Condition astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Sep 10, 2024
@PatrikLundell
Copy link
Contributor Author

The failed test is caused by master code, which contains the dubious practice of naming a variable the same as the type. It's probably caused by the rework of eggs and baby monsters, and should be dealt with separated from unrelated PRs.

@Maleclypse Maleclypse changed the title Code fix for #71971 Code fix for #71971 Water purification tablets only purify up to 4 water each Sep 10, 2024
@Maleclypse
Copy link
Member

I appreciate how you've done this and you've kept the commits intact better than I'd be able to do so. Since this is a fully superseding version inclusive of commits and I was able to compile and test this I'm going to merge this if that's ok.
water purify tablets work

@PatrikLundell
Copy link
Contributor Author

Do what you think is best. I don't care about credits etc. but @ZeroInternalReflection might.

I've done nothing to keep commits intact: it's all how it happened to end up when I followed the instructions on how to get hold of and upload the changed code.

@Maleclypse
Copy link
Member

Normally in situations like this we don't squash commits even if they had a bunch of gibberish commits in here to preserve credit. This doesn't have a lot of gibberish commits so it's an even easier merge.

@Maleclypse Maleclypse merged commit 1b831d8 into CleverRaven:master Sep 12, 2024
27 of 32 checks passed
@Maleclypse
Copy link
Member

Also apparently github recognized this was a continuation of the PR and counts it as merged as well

@Severhead
Copy link

Should water purification tablets not purify 6 each since they are designed to purify a one quart canteen?

@PatrikLundell PatrikLundell deleted the WaterPurification branch September 12, 2024 16:02
@Maleclypse
Copy link
Member

Should water purification tablets not purify 6 each since they are designed to purify a one quart canteen?

It is best to make an issue when you suspect changes need to be made to an already merged PR. Especially in this case where the author of this PR updated the mapgen parts of an earlier abandoned PR. Making a new issue will make it more likely that someone who will do something about the issue you are raising will see it.

@ZeroInternalReflection
Copy link
Contributor

I just saw the notifications for this getting merged and had to track down my GitHub password. Thanks for getting this over the finish line.

Now, hopefully there weren't any bugs left in my cod- Oh.
Yeah, I definitely should have split the 'consuming water you've backed out of consuming' bug into a separate PR. If I remember correctly, the code is fairly isolated, and reverting it until someone with better understanding of item_locations and the activity actor code can tackle it is probably the best option. Unfortunately, I'm not really set up to test it right now.

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` Code: Tests Measurement, self-control, statistics, balancing. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling EOC: Effects On Condition Anything concerning Effects On Condition Info / User Interface Game - player communication, menus, etc. Items: Food / Vitamins Comestibles and drinks [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display Spawn Creatures, items, vehicles, locations appearing on map
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants