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

Military Vehicle Group and Source Changes #12926

Merged
merged 4 commits into from Jul 15, 2015

Conversation

Projects
None yet
5 participants
@chaosvolt
Copy link
Contributor

commented Jul 13, 2015

I'm always leery of messing with the source, so I'm hoping I don't screw
up.

  1. Added a vehicle group for military vehicles. Cargo truck
    predominates, with other existing vehicles increasingly rare.
  2. Source tweaks in only two places. The parking lot result that spawns
    a cargo truck, and the turret result for military checkpoints.

I am assuming that the chicken walker and tank drone are much bulkier
than a pair or turrets and need a cargo truck to haul them, so those
spawn results are unchanged. I am likewise leaving the truck spawning in
the hazardous waste sarcophagus unchanged.

chaosvolt
Military Vehicle Group and Source Changes
I'm always leery of messing with the source, so I'm hoping I don't screw
up.

1. Added a vehicle group for military vehicles. Cargo truck
predominates, with other existing vehicles increasingly rare.
2. Source tweaks in only two places. The parking lot result that spawns
a cargo truck, and the turret result for military checkpoints.

I am assuming that the chicken walker and tank drone are much bulkier
than a pair or turrets and need a cargo truck to haul them, so those
spawn results are unchanged. I am likewise leaving the truck spawning in
the hazardous waste sarcophagus unchanged.
@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2015

Ah, I knew I'd mess something up. :V

EDIT: Scheisse. Am I missing additional inclusions, or is the problem something else?

chaosvolt added some commits Jul 13, 2015

chaosvolt
Update 2
1. Possible lack of a required file in the include list for
mapgen_functions, will see if that works.
chaosvolt
Update 3
1. Adding one more inclusion. If this isn't enough to fix it...
@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2015

Ah...hah...scheisse squared.

chaosvolt
Update 4
Meh. If someone can tell me I need to allow vehicle group spawns in
mapgen_functions.cpp, that'd be nice.  Going to fix that and save it for
later.

1. Removed failed attempt to spawn military-group vehicles in
mapgen_functions.
2. Added the vehicle group spawning to the chickenbot delivery
checkpoint. Pushing it a bit, but still more plausible than an APC
delivering a tank drone.
@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2015

Meh. For now it seems at least mapgen.cpp can be messed with.

@KiwiTrinsic

This comment has been minimized.

Copy link

commented Jul 15, 2015

From a quick glance, I'm guessing that the problem is that the veh_type variable is of vproto_id, while you are trying to assign a vgroup_id to it. That won't work. vgroup_id can handle individual vehicle types as well as groups, but vproto_id can only handle individual vehicle types. The map's add_vehicle function can accept either a vproto_id or a v_group_id without problem, which is why the changes in mapgen.cpp work.

You should be able to get the mapgen_functions.cpp change to work if you change veh_type from vproto_id to vgroup_id. Doing that you will need to change all the assigns to veh_type to vgroup_id. Example: vproto_id("bubble_car") needs to become vgroup_id("bubble_car")

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2015

Ah, I see. The error that kept showing up was "candidate function (the implicit copy assignment operator) not viable: no known conversion from 'string_id<VehicleGroup>' to 'const string_id<vehicle_prototype>' for 1st argument" if that helps any.

@narc0tiq

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

Right, and string_id<VehicleGroup> is the long-form name for vgroup_id, similarly string_id<vehicle_prototype>.

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2015

Hmm, I see. I could've sworn mapgen.cpp used both vproto_id and vgroup_id though.

EDIT: Yeah, even the military checkpoint spawning demonstrates it.

@KiwiTrinsic

This comment has been minimized.

Copy link

commented Jul 15, 2015

Yes, the map add_vehicle function can handle either a vproto_id or a vgroup_id. You still cannot assign a vgroup_id value to a vproto_id variable ;)

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2015

Hmm. So what I need to do is figure out how to change the add_vehicle function as defined in mapgen_functions.cpp and get it to take vgroup_id instead? Still unsure. :V

@KiwiTrinsic

This comment has been minimized.

Copy link

commented Jul 15, 2015

The way mapgen_functions.cpp is working right now is that it creates a variable veh_type of vproto_id. Then it will put the value into that variable. That is the part where you see veh_type = vproto_id( "military_cargo_truck" ), veh_type = vproto_id( "bubble_car" ), and so on. Because veh_type is a vproto_id variable, it can only be assigned a vproto_id value.

If you change veh_type to be a vgroup_id variable, you will then be able to assign it a vgroup_id value. This means that all the vproto_id values will have to be changed to vgroup_ids or you will have the same problem you are having now, except in reverse (trying to put a vproto_id value into a vgroup_id variable.)

@narc0tiq

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

...no. Your variable named veh_type from the line that reads veh_type = vgroup_id( "military_vehicles" ); has been declared as const vproto_id veh_type or similar. You're trying to put a vgroup_id in it. It's right there in the report from Jenkins at http://ci.narc.ro/job/Cataclysm-Jently/29392//consoleText.

It's no longer there in the latest commit, suggesting you did something to fix it. Most likely this part of the diff: https://github.com/chaosvolt/Cataclysm-DDA/commit/4a01afe2ff808e5f70475a684d001d7c9dd339e1#diff-59604ea2f6e3da1d8c5be9188964f1c9R1794.

@narc0tiq

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

Also, is the pull request builder busted again? Edit: Nevermind, I kicked it regardless.

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2015

Hmm. I undid my changes to mapgen_functions is why it isn't there, because it was breaking. But does it at least function correctly in mapgen.cpp?

If it's instead trying to spawn a VEHICLE named "military_vehicles" I'm going to be several different kinds of annoyed. ;-;

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2015

Also also, yeah it looks like something broke on the pull request builder, as update 4 hasn't been checked. O.o

@narc0tiq

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

Oh, no, I see what's going on now -- that "Update 4" is actually a reference:

chaosvolt referenced this pull request from a commit in chaosvolt/Cataclysm-DDA

Incidentally, it would be really nice if your commit titles were more informative. "Update 4" doesn't tell me anything, and if I'm looking at abbreviated git log output, the one line is all I have. Maybe you're not making enough commits if you feel the only possible short titles would be "Here's another update".

@KiwiTrinsic

This comment has been minimized.

Copy link

commented Jul 15, 2015

Everything looks fine to me in the mapgen.cpp code. It is calling directly to the add_vehicle function with the vgroup_id instead of using a variable so it won't have the same problems you are having in mapgen_functions.cpp.

@Coolthulhu Coolthulhu self-assigned this Jul 15, 2015

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2015

Incidentally, it would be really nice if your commit titles were more informative. "Update 4" doesn't tell me anything, and if I'm looking at abbreviated git log output, the one line is all I have. Maybe you're not making enough commits if you feel the only possible short titles would be "Here's another update".

Sorry about that. I usually list changes in the notes for it, but I probably should start titling commits with what I'm changing instead of numbering them. @_@

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2015

In any case, hope the mergening goes well.

Coolthulhu added a commit that referenced this pull request Jul 15, 2015

Merge pull request #12926 from chaosvolt/military-vehiclegroup
Military Vehicle Group and Source Changes

@Coolthulhu Coolthulhu merged commit db4a2b1 into CleverRaven:master Jul 15, 2015

1 check passed

default This has been rescheduled for testing as the 'master' branch has been updated.
@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2015

And thank you for the merge.

@chaosvolt chaosvolt deleted the chaosvolt:military-vehiclegroup branch Jul 16, 2015

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2015

Also @Coolthulhu if you know what I messed up in that initial commit, it would be nice to know. Was it a lack of additional includes like my guess was, what KiwiTrinsic guessed was the cause, or something else entirely?

@BevapDin

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2015

what I messed up in that initial commit,

It's what KiwiTrinsic guessed was the cause. You're assigning to the variable veh_type, which is of type vproto_id (vehicle prototype id). But you assign a value of type vgroup_id (vehicle group id).

Those are both ids, but they are ids of different things. It would similarly fail if you try to assign a martial art style (matype_id) or a monster group id (mongroup_id).

You can try to spawn from the group there directly (call m->add_vehicle( vgroup_id( ... ), ... ), just like the final add_vehicle call at the end of the loop).

Besides the whole if-else block should be moved to JSON, I think the vehicle group system allows that now.

@chaosvolt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2015

Hmm, I see. No doubt if I tried to puzzle out how to get that to work, I would've broke it even harder. ;w;

@KiwiTrinsic

This comment has been minimized.

Copy link

commented Jul 16, 2015

@BevapDin Yeah, I've taken another look at the code and it is basically trying to hard code a vehicle group. I'll look into ripping the whole thing out and replacing it with a json vehicle group.

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.