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

Driving Backwards #95

Merged
merged 14 commits into from
Sep 21, 2021
Merged

Conversation

DasCapschen
Copy link
Member

@DasCapschen DasCapschen commented Aug 21, 2021

Implement driving backwards. Solves #77.
There are quite a lot of changes to core driving logic, so you probably want to review this properly.
So far I have done some limited testing on the tutorial map, but it seemed to work fine there.

To Do:

  • Change station behaviour if train missed a station
  • Implement new feature to autopilot (has to "press" F once)
  • Enabling Debug Mode: Should set train to forward
  • Hint if you throttle up and the reverser is neutral
  • Controller mappings
  • Mobile version needs buttons for the reverser

Known issues:

  • Going across a green light at the end of a station turns it red, going backwards over it again does not turn it green again. This probably requires resetting "nextStation" or so. I haven't yet looked into station code. Will do soon.
  • Signals are handled before the train drives over sometimes (tested on Hainfurt -> Buchenhain). Seems to be in connectionwith the start distance on a rail in backward direction at beginning of a scenario
  • The go back message does not include the controls

@Jean28518
Copy link
Member

Thanks, I will review it!
That issue with the signal: I wouldn't reset any station variable because signals are not connected to stations in any way.

I would implement in this way: The train stores every overdriven signal in an array. If the train detects a signal in front, which is stored in this array, the train should turn that signal status to 1.

@DasCapschen
Copy link
Member Author

Okay, here is a short summary on how I implemented it:

  • I store all signal names and route positions in arrays when baking the route. These arrays are already sorted, so baked_route_signalNames[0] will be the first signal the player passes.
  • I keep track of the upcoming signal with an index nextSignalIndex
  • in check_signals I check the player position on route against the signal position on route, and if passed, nextSignalIndex += 1 (if driving backwards nextSignalIndex -= 1)
  • I then find the last signal (with index < nextSignalIndex) of type "Signal" and call giveSignalFree()

Looks like giveSignalFree() is the problem, it only works if the signal is a block signal, but the signal at the end of the station is not. I guess I should just use status = 1 right away.

Oh and I didn't yet check if you can drive past a station, then reverse into it and still service it. I think the game tells you "you drove past a station, go to the next one". So we would probably need to reset the station when reversing into it.

@Jean28518
Copy link
Member

Jean28518 commented Aug 22, 2021

Very nice and clean code!

At newer contributions I try to respect the following code guidelines: https://www.gdquest.com/docs/guidelines/best-practices/godot-gdscript/
Would be great if you could do this too in the future :-)

Could you please explain me the difference between distance and distanceOnRoute?

@DasCapschen
Copy link
Member Author

Oh, of course. I can do a bit of refactoring to follow the code guidelines, no problem. :)

Oh, distanceOnRoute is distance, I actually just renamed the variable, because there was already distanceOnRail and I got confused by what distance is, so I thought calling it distanceOnRoute makes it a bit more clear.

@Jean28518
Copy link
Member

Okay, yeah, good Idea!

Copy link
Member

@Jean28518 Jean28518 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is okay in general 👍

@DasCapschen
Copy link
Member Author

I tested it on Hainfurt now and also had the AI and Autopilot problems, that is fixed now :)
Station signals (blockSignal = false) will now also turn green.

I will look into signals being handled too early now 👍

@DasCapschen
Copy link
Member Author

Signals should no longer trigger early. This was indeed due to wrong initial distanceOnRoute, if forward = false for startRail.

@Jean28518
Copy link
Member

Why did you made the reverse state global? :)

@DasCapschen
Copy link
Member Author

Because it is accessed from multiple files, not just in Player.gd, and it's nicer to just write ReverserState.FORWARD than player.ReverserState.FORWARD, because really it's a constant, and not something intrinsic to the player.
Remember this is just the Enum for the possible values, the current value of the player is player.reverser and not global :)

@DasCapschen
Copy link
Member Author

I have now refactored the Station logic, which makes it easier to read and understand, and I've also changed it a little.

The station now only counts as missed if the whole train went past the station. (distance-distanceOnStationBeginning > stationLength changed to distance-distanceOnStationBeginning > stationLength + length)

Just like when the end of train is not yet in the station, when the train drives past the station a bit, whole_train_in_station is false, and the player is told to reverse into the station and try again.

Also fixed a bug where people got out of the train when the player passed the station signal and opened the doors, but the train was not yet fully in the station. (people would walk in mid air)

Copy link
Member

@HaSa1002 HaSa1002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only did brain compilation but looks good overall. Just some style stuff

src/Resources/Basic/Screens/ScreenReverser/Reverser.gd Outdated Show resolved Hide resolved
Comment on lines 4 to 6
FORWARD = 1,
NEUTRAL = 0,
REVERSE = -1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FORWARD = 1,
NEUTRAL = 0,
REVERSE = -1
FORWARD = 1,
NEUTRAL = 0,
BACKWARD = -1

if not forward:
currentSlope = - currentSlope
currentSlope = -currentSlope
if reverser == ReverserState.REVERSE:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if reverser == ReverserState.REVERSE:
if reverser == ReverserState.BACKWARD:

# drivenDistance = 0 - distanceOnRail
change_to_next_rail()
var driven_distance = speed * delta
if reverser == ReverserState.REVERSE:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if reverser == ReverserState.REVERSE:
if reverser == ReverserState.BACKWARD:

if distanceOnRail < 0:
change_to_next_rail()
var driven_distance = speed * delta
if player.reverser == ReverserState.REVERSE:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if player.reverser == ReverserState.REVERSE:
if player.reverser == ReverserState.BACKWARD:

forward = baked_route_direction[routeIndex]
if forward and (player.reverser == ReverserState.FORWARD):
distance_on_rail -= currentRail.length
if not forward and (player.reverser == ReverserState.REVERSE):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not forward and (player.reverser == ReverserState.REVERSE):
if not forward and (player.reverser == ReverserState.BACKWARD):

if not forward and (player.reverser == ReverserState.REVERSE):
distance_on_rail -= currentRail.length

if player.reverser == ReverserState.REVERSE:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if player.reverser == ReverserState.REVERSE:
if player.reverser == ReverserState.BACKWARD:


if not forward and (player.reverser == ReverserState.FORWARD):
distance_on_rail += currentRail.length
if forward and (player.reverser == ReverserState.REVERSE):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if forward and (player.reverser == ReverserState.REVERSE):
if forward and (player.reverser == ReverserState.BACKWARD):

@DasCapschen
Copy link
Member Author

@HaSa1002
I've merged some of your changes, but why all the changes of "Reverse" to "Backward"?

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 5, 2021

I believe, you rather drive backwards than driving reverse

@DasCapschen
Copy link
Member Author

Well, in a car the "Rückwärtsgang" is the "Reverse gear", so I'd say you are driving "in reverse", rather than "backwards".
It doesn't really make much of a difference, I think.

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 5, 2021

Then it is personal preference, I guess. I have no strong opinion about that as I am not a native speaker. Both is understandable IMO. Func fact: There is a comment in your code under a .REVERSE talking about backwards driving.

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 5, 2021

@DasCapschen
Copy link
Member Author

Will do 👍

@DasCapschen DasCapschen force-pushed the drive_reverse branch 3 times, most recently from c50333f to 35ec4af Compare September 6, 2021 20:50
@HaSa1002
Copy link
Member

HaSa1002 commented Sep 6, 2021

  • The go back message does not include the controls
  • No hint is shown if you throttle up and the reverser is neutral
  • In the middle on control board the directions are all displayed, but don't appear to have a function
  • R and F feel inverted, because R is above F but reverses
  • Controller mappings are missing (maybe, use DPAD, but we run out of buttons on controllers, so we need to find a better solution anyway)

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 6, 2021

The mobile version needs buttons for the reverser as well

@DasCapschen
Copy link
Member Author

DasCapschen commented Sep 6, 2021

* The go back message does not include the controls
* No hint is shown if you throttle up and the reverser is neutral

Will fix these, no problem :)

* Controller mappings are missing (maybe, use DPAD, but we run out of buttons on controllers, so we need to find a better solution anyway)

There will be #108 at some point, which might help reduce controller mappings.
I will check what bindings TSW2 and TS2021 use.

* The mobile version needs buttons for the reverser as well

This will need UI work. Don't want to overload a tiny mobile screen with more and more controls.
I think eventually the mobile will need to be a subset of the PC version (like forcing simple mode)

* In the middle on control board the directions are all displayed, but don't appear to have a function

There are the 3 possible directions and a knob next to it (might be hard to see, the cabin lighting is awfully dark) that points to the selected direction. This will be useful for #108.

* `R` and `F` feel inverted, because R is above F but reverses

This is a but of a weird one. Yes, R being above F, you'd think that R goes forward and F goes backwards, but also Reverse and Forward. So I'm not sure which is less confusing 😅

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 6, 2021

This will need UI work. Don't want to overload a tiny mobile screen with more and more controls.
I think eventually the mobile will need to be a subset of the PC version (like forcing simple mode)

I think #108 will fix that, but not the controller stuff, as you don't want to use a cursor through a controller. I am thinking about a radial or quick slot menu.

@DasCapschen
Copy link
Member Author

I think #108 will work fine on controllers. I was thinking of doing it like TSW2 (they have an xbox version on which this works fine):

you don't enable the mouse cursor and move that around, rather you have a little "crosshair" in the center of your screen and whatever control is below that gets selected. When you press the interact button, the view gets locked and moving the view stick / mouse around then moves the control.

On a mobile, of course you could do all that with your finger.

I am not opposed at all to a radial menu though, that is actually a really good idea! Even for PC.
Although it should go into it's own issue / PR :)

@DasCapschen
Copy link
Member Author

About the mobile HUD, I've just had an idea:
We have the up and down button, which are used to control the combined brake / acceleration lever.
What if we also make them control the reverser? Like this:

  • player is stopped (speed == 0)
  • player clicks arrow up -> Reverser changes to FORWARD -> click / hold arrow up more -> accelerate
  • player clicks arrow down -> Reverser changes to REVERSE -> click / hold arrow down more -> accelerate

And then while driving:

  • if FORWARD: arrow down -> brake
  • if REVERSE: arrow up -> brake

Basically making the arrows choose the direction you want to go in.

@DasCapschen
Copy link
Member Author

I would say this merge request is done.
Feel free to test it extensively before you merge it. :)

@Jean28518 Jean28518 changed the title WIP: Driving Backwards Driving Backwards Sep 16, 2021
Copy link
Member

@HaSa1002 HaSa1002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't close the doors via controller anymore, but not a blocker since we need to rework controller input anyhow.
LGTM.

@DasCapschen
Copy link
Member Author

You should still be able to close the doors. I changed to behaviour so that when you press "doors right" and the doors are already open, they will then close.
But I thought about reworking how the doors work anyways 😄

@HaSa1002
Copy link
Member

A good to know. But yeah not a problem really, as it was added in this dev version

@Jean28518 Jean28518 merged commit 34162ee into Libre-TrainSim:master Sep 21, 2021
@nalquas nalquas added the enhancement New feature or request label Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants