New vehicle light system is unusable

[Coolthulhu]

http://smf.cataclysmdda.com/index.php?topic=12809

I assume it is all temporary, but I'll make an issue as a reminder that the problem exists and has to be fixed.

Old light system allowed turning on the lights with 2 keypresses. New one only allows 2 keypress light toggles for specific settings.

It lacks a global light switch that would restore it to last known good state. Generally the vehicle lights are used in same configuration (say, controls only, floodlight only etc.) multiple times and changed only rarely. As such, the ability to restore the last state is more important than the ability to configure the state precisely.

For vehicle light system to be considered usable, it must allow toggling light state with 2 keypresses.
This operation must restore the old light state, not turn all lights on/off.

Similar problem exists for turrets: they also lack the ability to disable all of them with 2 keypresses and the ability to enable the selected turrets with 2 keypresses.

Finally, a minor note: it would be nice if the keybinds for vehicle controls weren't hardcoded but configurable.

[mugling]

Quoting from the forum post:

I'm in a recent experimental

The experimental branch only guarantees not to break existing saves. Features may be incomplete and will probably have flaws. Of course master also has all the exciting new features but you cannot have one without the other.

and the changes to this completely suck. Talk about pointless tedium. WHAT is being gained by this?

A large internal refactor of the vehicle code is underway. This is one of the more ugly sections of the codebase of which large amounts are entirely undocumented. This particular change is aimed at vehicle parts being toggled via their enabled field which in turn allows for simplification of power mechanics and finally of engines and fuel consumption.

I see zero upside here. Nothing gained

Refactoring is a big win. Good quality code hides fewer bugs, is easier to extend and encourages future contributions. Further vehicle features are often requested and a prior code cleanup is helpful in this respect.

much lost.

That PR leaves the vehicle lights functional but not as convenient as they could be. There is a balance between making sufficient progress and keeping a PR sufficiently small so as to be practical to review.

Additionally any regressions will be addressed in order of priority and not word count of complainant. For example #17261 was probably introduced by that PR and has precedence.

@coolthulu: the ability to restore the last state is more important than the ability to configure the state precisely

We could change bool enabled to enum class state which would allow a trivial implementation of toggle off then restore original state? As an added bonus the turret code could then use the same functionality as could anything else added in the future?

[Coolthulhu]

We could change bool enabled to enum class state which would allow a trivial implementation of toggle off then restore original state?

Not sure if enum is the best solution here, though. I'd rather see the state being opaque, only accessed through getters like part::is_enabled( const vehicle &parent ).
That way we could have an object belong to multiple states at once, force disabled state on 0 hp without changing the remembered state etc.

For a practical example, red and blue emergency lights could be enabled on alternating turns, without being hardcoded in light code.

[pingpong2011]

Something I noticed with vehicle parts, which might be relate-able: Certain things like the kitchen units have part-specific interactions (you have to use the part to act). Other things like the fridge or lights have control-specific interactions. I presume it's possible to have both part and control interactions, so maybe acting on the lights (or turrets) changes specific settings, but the controls have the more global on/off. If lights were categorized into headlights and "internal lights", that could be the global on/off, while the precise setting was modified by acting on the light.

[mugling]

enum class plus is_enabled accessor could work? In any case we have to store the state within the parts themselves

[Coolthulhu]

enum class plus is_enabled accessor could work? In any case we have to store the state within the parts themselves

Could work
As long as it is encapsulated nicely, it shouldn't cause problems during expansion.

Eventually it could be changed from enum to say, a set. For example, to have configurable states like:

• Alternator becomes enabled when battery power <90% ("high energy" state drops to "mid energy")
• When there is no player in the vehicle, all lights should shut down ("no passenger" state)
• When the player falls asleep on the controls (consciously, not by force), they automatically put the vehicle on "player asleep" state which turns off stereo and lights

But that's just for future goals. Still, that's why accessor is better than public field.

[mugling]

Effectively your proposing state is a flag field?

[Coolthulhu]

Not sure what do you mean by flag field.
Something like a bitset of flags? That would be one way to look at it.

[kevingranade]

On Jun 20, 2016 3:47 PM, "mugling" notifications@github.com wrote:

Quoting from the forum post:

I'm in a recent experimental

The experimental branch only guarantees not > to break existing saves.
Features may be
incomplete and will probably have flaws.

Where did you get this idea? Experimental should be playable at all times.
Do we make mistakes? Of course, but that's the goal. This is not
"incomplete", this is broken.

A large internal refactor of the vehicle code is
underway.

Please excuse my pedantry, but its a peeve of mine. Please stop calling
these refactors, they are not. A refactor provides new code with the same
features as the old code. Not similar, same.

Additionally any regressions will be
addressed in order of priority and not word
count of complainant. For example #17261
was probably introduced by that PR and has
precedence.

The caveat to this is that regressions have precedence over everything
else. This has not been happening recently, and the game has been getting
noticeably more unstable. You need to slow down and completely address any
breakage you've introduced before moving on to new features, or especially
subsystem rewrites, which are the source of most of the breakage.

[mugling]

Please excuse my pedantry, but its a peeve of mine. Please stop calling these refactors, they are not. A refactor provides new code with the same features as the old code. Not similar, same.

You are correct. The point I'm making is that the internal quality of the code, in particular documentation and correctness,is important but opaque to the end users. The OP's original post asked as to the benefits.

This has not been happening recently, and the game has been getting noticeably more unstable.

A multifactorial problem of which internal changes are indeed a large factor. I think almost everyone is now using the auto-update from @remyroy which increasingly magnifies the visibility of bugs.