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

Audit: Farming Vehicles + Forklift #72311

Merged

Conversation

ShnitzelX2
Copy link
Contributor

@ShnitzelX2 ShnitzelX2 commented Mar 10, 2024

Purpose of change

While making some new tiles for vehicle parts, I noticed a couple things about some of the vehicles:

  1. The Plow Tractor and Reaper Tractor had their plow and reaper parts mounted to the sides of a frame -- real-life tractors nearly always have them in front or in back of something.
  2. All three of the farming vehicle parts (plow, reaper, seed drill) use frames differently: the plow has one frame required for mount and another for crafting, the reaper only requires a frame for crafting, and the seed drill requires neither.
  3. The seed drill and plow crafting recipes require electric components despite these machines having nonelectric real-life analogs. Further, none of them required electronics skill to craft and have advanced variants that could utilize electricity if needed.
  4. The power usage of the farming vehicle parts should match horsepower needed to operate these vehicles in real life.
  5. The Forklift was obviously intended to have two arms on separate tiles, but this conflicted with the forklift vehicle part's description as a "pair of arms". Additionally, two tiles wide seems like too much for a bog-standard forklift.

Describe the solution

Point by point:

  1. CDDA doesn't really have a better way to make rows of crops aside from attaching three plows to a tractor, so I figured the plow/reaper could at least attach from the front/back of a frame like they would on a real-life tractor -- now they do. I left the seed drill alone since they could plausibly be placed side-by-side.
  2. Every farming vehicle part -- plow, reaper, seed drill, and their advanced variants -- now uses location: "structure". It's simpler and less arbitrary.
  3. All electronic components were removed from the farming vehicle parts. The plow is a moldboard plow, the reaper is something approximating an Adriance reaper, and the seed drill is a box of sheet metal with some pipes and wheels attached.
  4. The plow takes much more vehicle power to operate. It's digging 8" into the ground, after all, and farming shouldn't be free. The other parts' power costs are slightly increased.
  5. The newly-designed "Electric Forklift" is one tile wide with one forklift vehicle part (one pair of arms) and lacks heavy frames. The newly-named "Heavy-Duty Forklift" retains the original design.

Additionally, the Plow Tractor, Reaper Tractor, and Planter Tractor now spawn with items -- usually a basic tool and/or bottled beverage.

Describe alternatives you've considered

-I thought about making the reaper an electronic reaper, but in retrospect it would be too confusing to have only the reaper be electronic and then have an advanced variant on top of that.
-I thought about including a frame for the seed drill's crafting recipe, but it would be overkill.
-I'm still unsure about the advanced seed drill consuming electric power.

Testing

Spawned vehicles, checked power consumption, made sure game loaded, etc.

Additional context

First PR! Feel like this might be overdoing it for JSON edits, so feedback is appreciated.

Also, I need to redo the reaper sprite in my tileset PR. It's still an electric reaper.

most tractors have cup holders and glove compartments with tools
moved reaper vps to front and plow vps to behind frames; added some items
"Electric Forklift" is now one tile wide; renamed "Heavy-Duty" diesel forklift remains the same
plow/reaper/seed drill are now all nonelectric; more pipes/wheels required
advanced seed drill uses minimal amount of power
tying watts to horsepower wherever feasible
@github-actions github-actions bot added [JSON] Changes (can be) made in JSON Vehicles Vehicles, parts, mechanics & interactions Spawn Creatures, items, vehicles, locations appearing on map new contributor json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Mar 10, 2024
@PatrikLundell
Copy link
Contributor

I think you may have a compatibility problem, i.e. the issue that you change the mounting requirements for parts that may be in use in saves from before this change.
Old saves need to still work. That may be done by retaining old parts with their old specifications, obsoleting those, and then define new ones. It might be possible to add code to transform old designs to new ones, although that could be a bit tricky.

You have to follow the format for a PR (which is what the PR Validator test is complaining about). The PR summary section has to be present and follow the format the comment describes exactly since it's parsed by a script. The format is one of the listed code words followed by a descriptive comment, or the "None" code word without a comment.

@github-actions github-actions bot added Code: Tests Measurement, self-control, statistics, balancing. Game: Balance Balancing of (existing) in-game features. labels Mar 13, 2024
@ShnitzelX2
Copy link
Contributor Author

Sorry for the delay,

I think you may have a compatibility problem, i.e. the issue that you change the mounting requirements for parts that may be in use in saves from before this change. Old saves need to still work. That may be done by retaining old parts with their old specifications, obsoleting those, and then define new ones. It might be possible to add code to transform old designs to new ones, although that could be a bit tricky.

I tested the vehicle changes in a new build with an old save file, and there didn't appear to be any related errors. For the Plow Tractor, the plow vp is mounted to the frame when it shouldn't be, but the old save still works fine; you can drive the tractor, plow terrain, and even remove the plow vp if needed. Is this acceptable or should I obsolete the plow and advanced reaper?

You have to follow the format for a PR (which is what the PR Validator test is complaining about). The PR summary section has to be present and follow the format the comment describes exactly since it's parsed by a script. The format is one of the listed code words followed by a descriptive comment, or the "None" code word without a comment.

I fixed this (a silly error), and it also turned out that I forgot to run unit tests, so I updated the test data. Thanks!

@PatrikLundell
Copy link
Contributor

If old save vehicles work as intended you probably don't have a problem.

Copy link
Contributor Author

@ShnitzelX2 ShnitzelX2 left a comment

Choose a reason for hiding this comment

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

Author review

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 14, 2024
@Maleclypse Maleclypse merged commit 76fe0e3 into CleverRaven:master Mar 23, 2024
19 of 24 checks passed
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 Code: Tests Measurement, self-control, statistics, balancing. Game: Balance Balancing of (existing) in-game features. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions new contributor Spawn Creatures, items, vehicles, locations appearing on map Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants