Added wind turbine appliances #58370
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
[JSON]
|
The code failure on this JSON only PR is caused either by someone accepting a PR without verifying that the test doesn't fail, or by someone adding/modifying test criteria without fixing everything it would catch. Thus, it's unrelated to the PR. |
|
You're not going to add the makeshift wind turbine from the original author? |
|
I don't consider adding something that was removed for reasons I'm uncertain of without any knowledge whether it would have been approved or whether it would make sense to introduce a makeshift variant to be out of scope of this PR. I used the link provided in a post to the feature request as a guide to find which places I needed to modify and nothing more. |
|
Still wondering what's holding this one back from being added. |
|
No idea. Sometimes things just get overlooked for long enough to not be considered for looking at... |
|
A required test is failing, which makes it drop out of the filters which mergers generally use. |
|
Well, at least one test is almost always failing, with something like a 3% or so chance of not getting failures from the existing master code, both because people are more interested in adding new tests than maintaining old ones, and because some test makers deliberately make broken tests that incorrectly fail occasionally using the justification that each particular test only fails on a small percentage of usages (and without bothering to investigate each case where those tests trigger a failure). |
|
The general build matrix test fails on every PR currently |
|
Your PR cannot be merged if required tests are failing. There are 4 of them, and the false negative rate is very low for them. |
|
Ah, I didn't know some of the mandatory failing tests were more mandatory than others... |
github-actions
bot
added
the
BasicBuildPassed
|
General build matrix / Clang 12, Ubuntu, Tiles, ASan (pull_request) fails with this unintelligible mess: Run bash ./build-scripts/gha_test_only.sh
I doubt some JSON changes would cause the code to suddenly start to address illegal memory addresses. |
Fris0uman
reviewed
Aug 4, 2022
Co-authored-by: Fris0uman <[email protected]>
Co-authored-by: Fris0uman <[email protected]>
|
General build matrix / Clang 12, Ubuntu, Tiles, ASan (pull_request): Fails with what looks essentially the same as the last time... |
Co-authored-by: anothersimulacrum <[email protected]>
Co-authored-by: anothersimulacrum <[email protected]>
alef
pushed a commit
to alef/Cataclysm-DDA
that referenced
this pull request
Aug 6, 2022
* Added wind turbine appliances * trigger tests * trigger tests * Update data/json/construction.json Co-authored-by: Fris0uman <[email protected]> * Update data/json/construction.json Co-authored-by: Fris0uman <[email protected]> * Update data/json/construction.json Co-authored-by: anothersimulacrum <[email protected]> * Update data/json/construction.json Co-authored-by: anothersimulacrum <[email protected]> Co-authored-by: Fris0uman <[email protected]> Co-authored-by: anothersimulacrum <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
None
Purpose of change
Add wind turbines to the set of vehicle parts that can also be placed as appliances.
Fix #58353.
Fix #56780.
The PRs are duplicates.
Describe the solution
Added the necessary JSON entries.
It can be noted that the time to place them was increased to 30 minutes because these are fairly large things that need to be mounted, and mounted in a way that can withstand the forces placed upon them. Similarly, the removal time was set to 15 minutes, and the work was specified as medium rather than the standard light.
Describe alternatives you've considered
It could be argued that additional materials would be needed, as well as additional tools.
It could also be argued that the work load and times required should be the "standard" ones (or something else).
Originally water wheels were intended to be addressed as well, but testing showed that these require water to flow under them, and so would need some kind of framework on which to be mounted above water, and probably something on the shore as well to act as an anchor. This is not supported by the appliance framework, and the "old" vehicle implementation works well enough.
Testing
Hauled a large wind turbine up onto a roof and placed it next to a pre existing battery/solar panel appliance set. Verified that it claimed to produce power.
Spawned a (regular size) wind turbine and placed it next to the large wind turbine. Verified that it too produced power, and saw that the power was lower than that of the large wind turbine.
Additional context