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

Overhaul vehicle part installation [RDY] #16280

Merged
merged 36 commits into from Apr 20, 2016

Conversation

Projects
None yet
4 participants
@mugling
Copy link
Contributor

commented Apr 19, 2016

It's unrealistic that you can duct tape a 500kg engine onto a frame and drive away.

This PR adds JACK and LIFT tool qualities and encapsulates the players lifting ability as can_lift. The visitable interface is extended so that vehicle parts can provide tool qualities. A folding engine crane is provided as a demonstration item.

At present each level of JACK or LIFT has a capacity of 1000kg. LIFT is likely to be extended to require a steel chain and be an acceptable substitute for JACK. This might be reduced to 500kg per unit to improve the granularity noting @Coolthulhu's comments about vehicle jacks.

I'm going to extend this further so that players can construct cranes and other such vehicles that might be necessary for those wanting to work on huge vehicles.

Smaller vehicles remain repairable at the side of the road whereas larger vehicles may require workshop facilities for some tasks. This should hopefully balance out the vehicle parts more rather than just going for the biggest baddest engine you can find on day 1.

Comments are sought as to initial balance. The default STR 8 character can install parts up to 80kg so this is one major balance point.

@mugling mugling referenced this pull request Apr 19, 2016

Closed

Overhaul vehicle engines #16272

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2016

0799db1 provides for two different types of crane, a telescopic (foldable) 500kg variant mounted on a frame and a large rigid boom with 2000kg capacity that must be bolted to the outside of a vehicle that might fall of if driven into something.

I'm going to further extend this so that LIFT can also be used to raise a vehicle sufficiently off the ground to allow changing of tires on huge vehicles

int qual_jack = ceil( double( veh->total_mass() * 1000 ) / TOOL_LIFT_FACTOR );
has_jack = g->u.has_quality( "JACK", qual_jack ) ||
map_selector( g->u.pos(), PICKUP_RANGE ).has_quality( "JACK", qual_jack ) ||
vehicle_selector( g->u.pos(), 1, *veh ).has_quality( "JACK", qual_jack );

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Apr 19, 2016

Contributor

That 1 range is inconsistent with map selector range.

This comment has been minimized.

Copy link
@mugling

mugling Apr 19, 2016

Author Contributor

Deliberately so. Vehicle mounted cranes are somewhat fragile and the heavier variants need to be mounted as external protrusions. Forcing the player to maneuver to the target prevents exploits like hiding the crane in the middle of the vehicle.

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Apr 19, 2016

Contributor

Forcing the player to maneuver to the target prevents exploits like hiding the crane in the middle of the vehicle.

Sounds like it will do the opposite - make it so that big vehicles will always have the cranes inside crafting sections of the vehicle, because pulling out all the tools would be just too annoying to deal with.

This comment has been minimized.

Copy link
@mugling

mugling Apr 19, 2016

Author Contributor

I don't follow?

This comment has been minimized.

Copy link
@mugling

mugling Apr 19, 2016

Author Contributor

Cranes can't lift themselves (enforced in 4bb4ef8) so by definition you're already stood adjacent to another vehicle your working on. The only requirement is that that crane you want to use is also adjacent to you - your other crafting tools are found from the usual PICKUP_RANGE.

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Apr 19, 2016

Contributor

Cranes can't lift themselves

Then why limit them to 1 tile range?

If you need a separate vehicle to operate on main vehicle, you're going to build the simplest one possible. Protrusion won't make it vulnerable because it will be deconstructed when not used, not driven around.

Having to haul the crane around would be pure annoyance.

This comment has been minimized.

Copy link
@mugling

mugling Apr 19, 2016

Author Contributor

Then why limit them to 1 tile range?

Consistent behavior in all circumstances - cranes, forklift trucks and digger buckets should all have similar semantics. I'd like to model a realistic set of construction vehicles for works depots and the like. It doesn't make any sense why most mechanisms would work from 20 tiles away.

Having to haul the crane around would be pure annoyance.

The folding engine crane is sufficient for almost all parts installation tasks. If you're working on a sufficiently large vehicle you're already going to have to haul tools around due to PICKUP_RANGE.

If you need a separate vehicle to operate on main vehicle, you're going to build the simplest one possible.

Sure, but in another scenario you might also be using your main vehicle to salvage parts from another (much larger) vehicle/wreckage. Further ancillary uses are likely to develop with time.

Protrusion won't make it vulnerable because it will be deconstructed when not used, not driven around.

Adding/removing parts needs to have a non-trivial time penalty.

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Apr 19, 2016

Contributor

Consistent behavior in all circumstances

Inconsistent with crafting and map uses and annoying, for no good reason.

If you're working on a sufficiently large vehicle you're already going to have to haul tools around due to PICKUP_RANGE.

Nope - you will just need to work on the tiles that are further away. Same with crane - it has the range of an entire vehicle, you just need to stand next to it and the vehicle.

Sure, but in another scenario you might also be using your main vehicle to salvage parts from another (much larger) vehicle/wreckage.

If you're worried about safety, you can still do it with 1 tile range, by installing the crane 1 tile deep into the vehicle and a door next to it. Regardless of the placement, you still need to park pretty exactly next to the vehicle you want to operate on, which is a tile-counting kind of annoyance with no positive effects for the gameplay.

This comment has been minimized.

Copy link
@mugling

mugling Apr 19, 2016

Author Contributor

Having to haul the crane around would be pure annoyance.

Also, once you've backed the crane up to the target vehicle you can work on any of that vehicles tiles without need to further move the crane

This comment has been minimized.

Copy link
@mugling

mugling Apr 19, 2016

Author Contributor

Inconsistent with crafting and map uses

A lot of stuff uses adjacent tiles and those which don't typically require line-of-sight to the tool, eg. WELDRIG etc so are in many cases less than PICKUP_RANGE anyway

Regardless of the placement, you still need to park pretty exactly next to the vehicle you want to operate on

Yes, but this is intentional. I don't want cranes to become another mandatory item you must craft and bolt onto your vehicle then forget about. There's no point in differing types if their placement isn't significant.

If you're worried about safety, you can still do it with 1 tile range, by installing the crane 1 tile deep into the vehicle and a door next to it

That's an interesting tactic but would make maneuvering more difficult. Inside cities space can be especially tight so you'd be passing up on scavenging parts from some military wreckages that you now can't get the angle on.

because pulling out all the tools would be just too annoying to deal with.

The only difference of cranes having radius 1 is that you have to successfully maneuver your vehicle to the target. This is very much a feature. The argument about tools is null in that you already have to move your tools every PICKUP_RANGE tiles. I can't see any other meaningful differences but I'm open to further discussion.

@@ -454,8 +469,7 @@ task_reason veh_interact::cant_do (char mode)
case 'c': // change tire
valid_target = wheel != NULL;
///\EFFECT_STR allows changing tires on heavier vehicles without a jack
has_str = g->u.get_str() >= int(veh->total_mass() / TIRE_CHANGE_STR_MOD);
has_tools = has_wrench && (has_jack || has_str) && has_wheel;
has_tools = has_wrench && ( g->u.can_lift( *veh ) || has_jack ) && has_wheel;

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Apr 19, 2016

Contributor

Swapping the bool and function invocation around would skip a potentially expensive invocation.

This comment has been minimized.

Copy link
@mugling

mugling Apr 19, 2016

Author Contributor

Good point, most cases will short-circuit on has_wheel

@@ -1142,9 +1106,14 @@ bool veh_interact::can_remove_part(int veh_part_index, int mech_skill, int msg_w
if (g->u.has_trait("DEBUG_HS")) {
return true;
}

if( is_wheel && !( g->u.can_lift( *veh ) || has_jack ) ) {

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Apr 19, 2016

Contributor

You're checking g->u.can_lift( *veh ) || has_jack quite a bit, you could extract it into a separate variable for readability.

This comment has been minimized.

Copy link
@mugling

mugling Apr 19, 2016

Author Contributor

This is now mostly refactored

mugling added some commits Apr 19, 2016

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2016

6f7bddd lets vehicle cranes assist in wheel changes. As the entire vehicle does not need to be lifted lever action results in their rating being increased by a factor of 3

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2016

I'm not sure how to balance this for huge vehicles. Possibly by capping effective vehicle weight when calculating JACK requirements?

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2016

Allow jacks mounted on a single vehicle to stack, but not with handheld ones.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2016

I'd rather avoid any extra complexity. Supporting arbitrarily large vehicles is a requirement but that support doesn't necessarily have to be balanced (20-tile wide 100-tonne vehicles aren't themselves balanced). A cap would be the simplest option whilst preserving balance for everyone else, I'm just not sure where to set it at, possibly 6000kg?

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2016

In real life, a vehicle huge enough to be unliftable with a crane would probably be parked above a pit before repairs, with the wheel to be replaced hanging in air.

6f7bddd lets vehicle cranes assist in wheel changes. As the entire vehicle does not need to be lifted lever action results in their rating being increased by a factor of 3

Wheels are the only part that fully utilizes jacks and so values should be centered around them. Instead of multiplying wheel efficiency by 3, multiply everything by 3 and then divide by 3 for non-wheels.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2016

Wheels are the only part that fully utilizes jacks and so values should be centered around them. Instead of multiplying wheel efficiency by 3, multiply everything by 3 and then divide by 3 for non-wheels.

The current implementation requires scaling by 3 exactly one (in veh_interact::cache_tool_availability). We might extend LIFT in future to include more uses so I'd prefer to keep the units as they are.

"broken_color": "light_blue",
"difficulty": 5,
"durability": 200,
"qualities": [ [ "LIFT", 4 ] ],

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Apr 19, 2016

Contributor

Why not specify JACK explicitly?

This comment has been minimized.

Copy link
@mugling

mugling Apr 19, 2016

Author Contributor

That's a very good point

double qual = double( std::min( veh->total_mass(), 6000 ) * 1000 ) / TOOL_LIFT_FACTOR;

// when lifting vehicles to change wheels lever action reduces the workload
has_jack = max_lift >= ceil( qual / 3 ) ||

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Apr 19, 2016

Contributor

Could be replaced by just giving cranes JACK of LIFT * 3, which would be wayyy less ugly than that.

@mugling mugling changed the title Overhaul vehicle part installation [CR] Overhaul vehicle part installation Apr 19, 2016

@Coolthulhu Coolthulhu self-assigned this Apr 20, 2016

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

Segfault when viewing wheel in install menu

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

Segfault when viewing wheel in install menu

Can you be more specific? I'm guessing you don't have a stack trace?

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

No stack trace, I could recompile later as debug but it will take a while.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

What are the exact steps?

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

  • Spawn car
  • 'e'xamine car
  • Crash

Could be affected by:

  • Low strength value (zero division?)
  • Debug hammerspace mutation
@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

Is it reproducible without DEBUG_HS?

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

Yes.

Also, seems to only affect wheels. It's not zero division because it happens at 3 str too.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

How does those steps to examine the car relate to wheels?

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

Doesn't seem to crash when I am looking at non-wheel parts. Crashes when a wheel part is selected, regardless of whether I have the wheel I want to install or not.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

The tool requirements are also off, though it may be caused by some earlier PR. I tried to install an aisle light and noticed that it no longer colors the screwdriver/duct tape requirement. This is confusing, as it looks like I have all I need, but pressing enter does nothing.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

Doesn't crash when I'm looking at a wheel to remove. Though the strength requirements are insane - 102 strength for a 1 ton car?

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

The tool requirements are also off, though it may be caused by some earlier PR

Let me try it against the current master and see if I can reproduce

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

This one is probably caused by this PR though: removing some wheels (portable generator has small wheels) doesn't list strength requirements at all, despite still requiring strength.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

Crashes when a wheel part is selected, regardless of whether I have the wheel I want to install or not.

Suggests the problem is in can_install_part

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

Have candidate fix for all of the above

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

Try with f537def

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

Damn that excavator is fucking fast

350 km/h excavator charge, woo

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

350 km/h excavator charge, woo

At some point we really need to get round to properly scaling those units

Damn that excavator is fucking fast

The original plan was to later extend it so the crane could demolish walls but I suppose ram-raiding is always an option

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

That lifting equipment display is kinda confusing. You have to check your individual items to see how much you can actually lift, having less will just color the "lifting equipment" red. Would be cool if it described it as "lifting equipment with quality x" or something like that.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

Though the strength requirements are insane - 102 strength for a 1 ton car?

The algorithm's working correctly (10kg/STR). Whilst I could cap lift_strength it's somewhat academic as I'd like part 2 to be the extension to use your previously proposed soft caps.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

Would be cool if it described it as "lifting equipment with quality x" or something like that.

That requires a little refactoring to expose max_jack as opposed to has_jack. I also want to start splitting component, skill and tool requirements onto separate lines. Again these are all part 2

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2016

When starting new projects, it's better to keep the changes low, to avoid ruining saves. That 10kg/str is very low, 50kg/str would be safer.

@mugling

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

When starting new projects, it's better to keep the changes low, to avoid ruining saves.

Fair enough, c382737 defangs it for now until I implement the next part

@Coolthulhu Coolthulhu merged commit e50cbb7 into CleverRaven:master Apr 20, 2016

@WizardOfOoo

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2016

Forgive me if this is the wrong place. But, I think installing the boom crane on an empty space is counter intuitive. Could it be changed so that the boom crane can be installed on a frame like virtually every other part?

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.