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

Forklifts! #61047

Merged
merged 18 commits into from
Sep 19, 2022
Merged

Forklifts! #61047

merged 18 commits into from
Sep 19, 2022

Conversation

Jarewill
Copy link
Contributor

Summary

Content "Updates forklifts"

Purpose of change

I love forklifts! However I felt their representation in the game wasn't correct.
For one, in game forklifts are made out of the usual car frames/boards instead of proper heavy duty ones.
Forklifts are meant to be tools that lift and carry really heavy things, so having a simple sheet metal for a roof instead of a heavy duty one that wouldn't collapse on the driver was really off.

Describe the solution

I based all the changes on my own forklift.
For one, it's now completely heavy-duty with frames, quarterpanels and roofs.

The back quarterpanels were also replaced with a new counterweight part, which is essentially a heavier quarterpanel.
A single counterweight is 750kg, which is doubled since there are two installed, again this was based on my own forklift's counterweight.

Wheels I was very unsure about, my forklift has front wheels that are bigger than the back ones and only the back ones are steerable.
I measured them with tape and the front ones ended up being roughly the same diameter as my car's, so I went with a car wheel for the front and a small wheel for the back.

I also gave them headlights, but I'm not sure about that.
I don't know if there's any item to represent a caged spotlight in the game however.

There's also a diesel variant to the electric one using the newly added I4 diesel engine!

Describe alternatives you've considered

Not adding a counterweight item.

Testing

Spawned the forklifts and drove a little.
Spawned a military helipad and confirmed both variants spawn there.

Additional context

I need help with this.
The only place where I know a forklift can spawn is the military helipad, so in terms of spawn groups this PR is still incomplete.
There are frankly way too many individual mapgen files to go through individually to check for forklift spawns, so any pointers to where they can spawn would be very helpful.

@github-actions github-actions bot added [JSON] Changes (can be) made in JSON Map / Mapgen Overmap, Mapgen, Map extras, Map display Vehicles Vehicles, parts, mechanics & interactions json-styled JSON lint passed, label assigned by github actions labels Sep 16, 2022
@github-actions
Copy link
Contributor

Spell checker encountered unrecognized words in the in-game text added in this pull request. See below for details.

Click to expand
  • A half-height heavy metal wall. Commonly used in forklifts to balance them when lifting heavy things.
  • counterweights

This alert is automatically generated. You can simply disregard if this is inaccurate, or (optionally) you can also add the new words to tools/spell_checker/dictionary.txt so they will not trigger an alert next time.

@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Sep 16, 2022
@Golfavel
Copy link
Contributor

Did a quick search, these should be all of the forklifts spawns:

  • Recycle centers
  • Apple orchards (this I can confirm having seen)
  • aircraft_carrier_z1
  • Abandoned warehouses
  • Helipads
  • Lumberyards
  • lab_cargo_surface
  • lab_1x1_security
  • mil_base_z0
  • Malls (roof and second floor)

@Jarewill
Copy link
Contributor Author

Thanks for that!
I'll get to updating those locations.

@Jarewill
Copy link
Contributor Author

So if I understand this correctly, making the forklifts heavier messed up the vehicle drag tests?
How would I go about fixing that?
And also fixing the spelling errors because (at least according to google) they are spelled correctly.

@mqrause
Copy link
Contributor

mqrause commented Sep 16, 2022

So if I understand this correctly, making the forklifts heavier messed up the vehicle drag tests? How would I go about fixing that?

The test has some precalculated expected values here:

test_vehicle_drag( "forklift", 0.565988, 1.096827, 517.625000, 8356, 8668 );

The test failure prints a replacement of the expected values: test_vehicle_drag( "forklift", 0.565988, 6.504838, 3683.790625, 7182, 7507 );
You just need to switch them out (assuming all your changes make sense).

@Jarewill
Copy link
Contributor Author

I am pretty sure they make sense as forklifts are pretty heavy-duty machines.
They need to weight a lot in order to balance out the things they are supposed to lift, otherwise they would turn over from the unbalanced weight of the object.

Sadly I still have no clue how to compile the game and all my attempts have been in vain, so I'm hoping that the code you provided will be a fix.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. labels Sep 16, 2022
@mqrause
Copy link
Contributor

mqrause commented Sep 16, 2022

That's honestly nothing you should have to compile the game for. Json changes are meant to be just json changes.
This should just work, though.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Sep 17, 2022
@Jarewill Jarewill marked this pull request as ready for review September 18, 2022 16:33
@dseguin dseguin merged commit 8d6c0ef into CleverRaven:master Sep 19, 2022
@propurty
Copy link

propurty commented Sep 20, 2022

Not sure if the lab cargo location listed above includes a TCL. But forklifts do spawn there, just in case the TCL wasn't covered.

By the way great PR, I was literally thinking the same exact thing since I have a forklift too. Although mine is old and beat up, everything you said is still accurate in describing mine.

@Jarewill
Copy link
Contributor Author

Not sure if the lab cargo location listed above includes a TCL. But forklifts do spawn there, just in case the TCL wasn't covered.

Hah, fellow forklift enthusiast!
Yes, the lab locations do include TCLs, which causes the new forklift variant to spawn there as well.

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 [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. [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 Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants