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

Farm Tractor Changes #13660

Merged
merged 3 commits into from Sep 28, 2015

Conversation

Projects
None yet
4 participants
@chaosvolt
Copy link
Contributor

commented Sep 27, 2015

Remind me to rename the Tankmod tractor (heavy tractor, maybe) and possibly give it variants, now that we have tractors that DO something in mainline. I might do that in this PR actually, alongside making use of the new farming vehicle group...

  1. Changed part slot numbers so that 0/0 isn't an unboardable location, fixing an error that cropped up when spawning tractors via debug.
  2. Changed vehicle layout to cover the exposed diesel tank and otherwise clean up the appearance (this might need further tweaks).
  3. Replaced V8 engines with V6. Even then it's still pushing 100+ MPH safe speed. Couldn't find reliable info on engine differences between smaller open-top tractors and enclosed-cab models.
  4. Fixes advanced seed drill part not using the advanced seed drill item to build.

chaosvolt added some commits Sep 27, 2015

chaosvolt
Farm Tractor Changes
1. Changed part slot number so that 0/0 isn't an unboardable location,
fixing an error that cropped up when spawning tractors via debug.
2. Changed vehicle layout to cover the exposed diesel tank and otherwise
clean up the appearance (this might need further tweaks).
3. Replaced V8 engines with V6. Even then it's still pushing 100+ MPH
safe speed. Couldn't find reliable info on engine differences between
smaller open-top tractors and enclosed-cab models.
chaosvolt
Allows tractors to actually SPAWN
* Far as I can tell, the "chance" entry seems to be required.
chaosvolt
@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2015

And add http://smf.cataclysmdda.com/index.php?topic=11325.0 to my to-to list. @kevingranade, this (and the earlier redundant vehicle.cpp definitions) is why the majority of PRs tend to be assigned to another dev instead of self-merged, even when it's another collaborator doing the pull request. Having a second pair of eyes during the merging process is a good thing.

EDIT: And for an issue I don't think I can fix, plows don't seem to be churning up the ground like they should.

@BevapDin

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2015

EDIT: And for an issue I don't think I can fix, plows don't seem to be churning up the ground like they should.

Have you turned it on? Did you drive across diggable terrain (e.g. dirt or grass)?

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2015

The controls menu listed no option to enable/disable the plows.

EDIT: Huh, now I'm getting that option. Strange.

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

Okay, this is just plain weird.

dafuq

I'd initially encountered an issue when testing in build 3725, and vehicle plows were FUBAR both before and after adding the changes this PR made.

Then, as I mentioned later today, testing with a later build (need to start up my laptop to check exactly which build) revealed the plow was working both before and after dumping the PR's changes into it.

I didn't see any mention of recent PRs unfucking plow functioning, so I had no idea what caused it and what fixed it.

Rivet-the-Zombie added a commit that referenced this pull request Sep 28, 2015

@Rivet-the-Zombie Rivet-the-Zombie merged commit bd47285 into CleverRaven:master Sep 28, 2015

1 check passed

default
Details
@kevingranade

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

@kevingranade, this (and the earlier redundant vehicle.cpp definitions) is why the majority of PRs tend to be assigned to another dev instead of self-merged, even when it's another collaborator doing the pull request. Having a second pair of eyes during the merging process is a good thing.

That's fucking rude man, and you're wrong about it. I wrote the damn policy that people don't merge their own PRs, and I follow it. I made a mistake while merging BevapDin's fixes to the branch. I had previously posted the PR for review, and got a "this works, except for bla bla" from BevapDin. After I said something about being too busy to get back to it right away, BevapDin posted the fixes to the issues he had raised. I pulled, merged, built and tested those changes. In the meantime someone merged something else, then I merged again and pushed. I should have rebuilt, I should have rebuilt and tested, but it was midnight and I'd been working on it for several hours already (the merges were a mess because of the way the original developer did their commits, not "wrong", but it makes merging hard). Yes that was a mistake, but it's not cool for you to call me out on it multiple times in public. I've cleaned up your messes a number of times, but I don't chew you out for it, and I expect some common courtesy here.

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

Sorry about that. I shouldn't have put it that way, yes.

Admittedly I can be just as prone to mocking my OWN mistakes on commits. >.>

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

And...given it was had gone through so many changes and had already changed hands at least once, it's understandable. I shouldn't have gotten pissy about it.

@chaosvolt chaosvolt deleted the chaosvolt:farm-tractor-changes branch Sep 28, 2015

@BevapDin

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

I'd initially encountered an issue when testing in build 3725, and vehicle plows were FUBAR both before and after adding the changes this PR made.

Build 3725 does not have plows or any of the farming parts. It's based on 55864e9, which is right before the farming stuff PR was merged. Whatever plow/reaper/planter part you have tested there could not have worked. The next build (http://ci.narc.ro/view/Cataclysm-DDA/job/Cataclysm-Matrix/3726/) has the farming parts.

Then, as I mentioned later today, testing with a later build (need to start up my laptop to check exactly which build) revealed the plow was working both before and after dumping the PR's changes into it.

Yes, because the farming PR was merged in the meantime and is therefor included in the "later build". Next time, please state the versions you have issue with and what you have changed in it.

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

Huh. That is weird. I could've sworn I tested it before dumping my changes into the game. More importantly, that test didn't include any vehicle part tweaks, so if it was unedited it should've puked up an error about the relevant parts not existing.

I can only assume I dumped some other JSON folders in it for testing before and utterly forgot. ._.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.