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

Railway crossing behaviour for peeps and vehicles #6998

Merged
merged 13 commits into from
Jun 11, 2018

Conversation

JeroenDStout
Copy link
Contributor

This is the code from my experimental branch which handles the vehicles and peeps interacting with crossings:

cross

This works by having vehicles 'reserve' tiles by giving path elements a flag (that is normally never set), while peeps check whether the path they are about to walk on has that flag.

I cannot work too much on this myself, so here are my thoughts:

  • Right now the debug 'grey' overlay is on by default to clarify which tiles are reserved
  • Crossings that are small enough are reserved together, but long stretches (like a city tram) are reserved with a maximum ahead of the train
  • Vehicles claxon when reserving a new crossing, although erroneously for long stretches the claxon is repeated every tile
  • Peeps prevent themselves 'stalling' by being allowed to keep moving when already on the tile that would block them, but while moving along a crossing (not orthogonal to it) they become blocked
  • Peeps can be given a better current action to prevent them being in the 'half step' sprite; although the current look feels more dynamic than the 'standing straight' queue sprite
  • Multiple vehicles in close succession may break one-another's reserves. This can be fixed by doing the vehicle update in two passes; remove reserves before updating all vehicles (checking the same progression variable), the other in the current place to re-reserve the tiles.
  • Reserving/freeing is not perfect and probably can be refined

I see two directions for fixing peep behaviour for city tram style crossings:

  • Make peeps leave a track when a vehicle reserves it, meaning they may break their current pathing but at least will get out of the way. This can be done by cheekily changing their target.
  • Do not allow peeps to walk 'along' multiple tracks. I.e., if they are on a tile with a crossing, do not allow the pathfinding to walk in the direction following that track. This may break AI pathing and does not mimic IRL city behaviour too well.

rct_tile_element * path_element = map_get_path_element_at(peep->destination_x / 32, peep->destination_y / 32, peep->next_z);
if (path_element && path_element->flags & TILE_ELEMENT_FLAG_BLOCKED_BY_VEHICLE)
{
if (!(peep->x >> 5 == peep->destination_x >> 5 &&
Copy link
Member

Choose a reason for hiding this comment

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

What are the lower 5 bits? Can they differ? In the line above you also divide destination_[xy] by 32 already, so you could just reuse the value here.

Copy link
Member

Choose a reason for hiding this comment

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

These are "big"/"sprite" coordinates, which are 32 times as big as regular coordinates. This code checks if both refer to the same tile, rather than the exact same spot within that tile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I suppose it depends on coding conventions whether to re-use,, I usually prefer to leave it to the compiler to do optimisations on that scale.

Copy link
Member

Choose a reason for hiding this comment

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

@Gymnasiast unless (x & 0x1f) != (destination_x & 0x1f), x >> 5 == destination_x >> 5 implies x == destination_x and you've just wasted 4 instructions shifting things for no reason. Hence my question:

What are the lower 5 bits? Can they differ?

@JeroenDStout leaving it to the compiler is absolutely fine, but a) you make it harder for the compiler to deduce the meaning of it by using / and >> b) you still pay the full price in unoptimised builds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@janisozaur That is a very good point!

}
}

void vehicle_claxon(rct_vehicle * vehicle)
Copy link
Member

Choose a reason for hiding this comment

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

argument should be const

{
rct_ride_entry* rideEntry = get_ride_entry(vehicle->ride_subtype);
audio_play_sound_at_location((rideEntry->vehicles[0].sound_range == 3)? SOUND_TRAIN_WHISTLE : SOUND_TRAM, vehicle->x, vehicle->y, vehicle->z);
}
Copy link
Member

Choose a reason for hiding this comment

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

missing newline

@@ -9890,3 +9899,150 @@ void vehicle_invalidate_window(rct_vehicle * vehicle)

window_invalidate(w);
}

void vehicle_update_crossings(rct_vehicle * vehicle)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the argument could be const? It's passed down to vehicle_get_{head|tail}() which should be const already and vehicle_claxon() which I don't know

@@ -8249,6 +8254,8 @@ static bool vehicle_update_track_motion_forwards(rct_vehicle * vehicle, rct_ride
uint16 trackTotalProgress = vehicle_get_move_info_size(vehicle->var_CD, vehicle->track_type);
if (regs.ax >= trackTotalProgress)
{
vehicle_update_crossings(vehicle);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the correct place to put this. Should this not be after a movement to a new track piece? I would also like to see this guarded by a flag so that only transport rides enter this code. There is no point having rollercoasters enter this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point, moving it to after moving would mean doing one offset calculation for the reservation fewer. The ordering here is an artefact from me snipping the code out of other things.

@@ -8594,6 +8601,8 @@ loc_6DBA33:;
regs.ax = vehicle->track_progress - 1;
if (regs.ax == -1)
{
vehicle_update_crossings(vehicle);
Copy link
Contributor

Choose a reason for hiding this comment

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

As above.

{
if (!playedClaxon && 0 == (tileElement->flags & TILE_ELEMENT_FLAG_BLOCKED_BY_VEHICLE))
{
vehicle_claxon(vehicle);
Copy link
Contributor

Choose a reason for hiding this comment

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

vehicle_claxon or is it a crossing claxon? I've not actually tested this so it might be a silly question.

void vehicle_claxon(rct_vehicle * vehicle)
{
rct_ride_entry* rideEntry = get_ride_entry(vehicle->ride_subtype);
audio_play_sound_at_location((rideEntry->vehicles[0].sound_range == 3)? SOUND_TRAIN_WHISTLE : SOUND_TRAM, vehicle->x, vehicle->y, vehicle->z);
Copy link
Contributor

Choose a reason for hiding this comment

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

Most vehicle sound effects are performed by the vehicle sound code rather than played directly. I'm not sure if that is the approach that should be done here but if it is I suggest using the sound2_flags (a param that is labelled in my PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not know about the flags while writing this - letting the flags handle it sounds much more consistent. The function was mostly meant as a general way to just get a sound played for any reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's okay i only found them yesterday :)

@duncanspumpkin
Copy link
Contributor

What happens when you double close the ride with the crossing turned on? Will it be constantly locked out. I see your using the indistructable flag to do this. Will that effect destructing the ride?

@Gymnasiast
Copy link
Member

@duncanspumpkin When destructing the ride, it will check a flag on the Ride itself. The indestructible flags on the track elements themselves are not checked in that case.

As for reusing the flag: as long as crossings stay confined to non-roller coasters, it's acceptable to use them - as long they don't linger.

@duncanspumpkin
Copy link
Contributor

Yeah sorry made a mistake there the indistructable flag is a red herring as the flag is put on the path element. The question still stands what happens when you remove the ride when it is blocking the path as i see nothing that would reset it.

@JeroenDStout
Copy link
Contributor Author

JeroenDStout commented Jan 11, 2018

@duncanspumpkin The bits are never used for paths, only for tracked (?) rides. I did name the same bit-flag a different (TILE_ELEMENT_FLAG_BLOCKED_BY_VEHICLE) to differentiate between the two use cases.

You are very right about the ride being closed/removed leaving the paths blocked. It's my mistake not adding that to the to-do notes :). I suppose in these cases a function would need to be called (remove blocked tiles for ride or so) that just once loops through the entire ride to release all the paths.

@tobyhinloopen
Copy link
Contributor

Is this still being worked on? It looks like a great feature!

@AaronVanGeffen
Copy link
Member

I've rebased this PR to account for the recent peep refactor. This will still need to be finished, however. It hasn't been updated since its initial publication…

@RobertWHurst
Copy link

Any chance this will get added at some point soon?

@AaronVanGeffen
Copy link
Member

Absolutely, but someone will need to finish it first, as I noted in my comment two weeks ago.

@tobyhinloopen
Copy link
Contributor

@JeroenDStout Need some help fixing things?

@tobyhinloopen
Copy link
Contributor

Thank you, @IntelOrca ! Excited to see progress 👍

// Check if vehicle is blocking the destination tile
auto curPos = TileCoordsXYZ(CoordsXYZ { x, y, z });
auto dstPos = TileCoordsXYZ(CoordsXY { destination_x, destination_y }, next_z);
if (curPos.x != dstPos.x || curPos.y != dstPos.y)
Copy link
Member

Choose a reason for hiding this comment

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

We should probably overload the == operator for TileCoordsXYZ so we can compare them directly.

@IntelOrca
Copy link
Contributor

I have resolved the following issues:

  • Added a debug paint option to show blocked tiles.
  • Unblock all blocked tiles for a ride when its vehicles are removed.

All other issues still remain but can be fixed separately. Separate issues can be also be made for them. I suggest this is merged as soon as we release 0.2.0.

@Gymnasiast Gymnasiast added this to the Post v0.2.0 milestone Jun 7, 2018
@AaronVanGeffen AaronVanGeffen added the network version Network version needs updating - double check before merging! label Jun 8, 2018
@AaronVanGeffen AaronVanGeffen added the changelog This issue/PR deserves a changelog entry. label Jun 10, 2018
@IntelOrca IntelOrca merged commit b043428 into OpenRCT2:develop Jun 11, 2018
@Umdlye Umdlye mentioned this pull request Jun 15, 2018
16 tasks
janisozaur added a commit that referenced this pull request Aug 26, 2018
- Feature: [#5993] Ride window prices can now be set via text input.
- Feature: [#6998] Guests now wait for passing vehicles before crossing railway tracks.
- Feature: [#7658] Add option to always use system file browsing window.
- Feature: [#7694] Debug option to visualize paths that the game detects as wide.
- Feature: [#7713] The virtual floor now takes land ownership rights into account.
- Feature: [#7771] Danish translation.
- Feature: [#7797, #7802, #7821, #7830] Add sprite font glyphs for Danish, Norwegian, Russian, Turkish, Catalan and Romanian.
- Feature: [#7848] Add a master volume slider to audio options screen.
- Feature: [#7868] Placing scenery while holding shift now scales appropriately with zoom levels.
- Feature: [#7882] Auto-detect Steam and GOG installations of RCT1.
- Feature: [#7885] Turkish translation.
- Fix: [#3177] Wrong keys displayed in shortcut menu.
- Fix: [#4039] No sprite font glyph for German opening quotation mark.
- Fix: [#5548] platform_get_locale_date_format is not implemented for Linux.
- Fix: [#7204] Object source filters do not work for RCT1, AA and LL.
- Fix: [#7440] Memory leak. All system memory used.
- Fix: [#7462] Guest window goes beyond the map edge on a spiral slide.
- Fix: [#7533] Screenshot is incorrectly named/file is not generated in CJK language.
- Fix: [#7628] Always-researched items can be modified in the inventory list.
- Fix: [#7643] No Money scenarios with funding set to zero.
- Fix: [#7653] Finances money spinner is too narrow for big loans.
- Fix: [#7673] Vehicle names are cut off in invention list.
- Fix: [#7674] Rides show up as random numbers in guest's ride list.
- Fix: [#7678] Crash when loading or starting a new game while having object selection window open.
- Fix: [#7683] 'Arbitrary ride type' dropdown state is shared between windows.
- Fix: [#7697] Some scenery groups in RCT1 saves are never invented.
- Fix: [#7711] Inverted Hairpin Coaster allows building invisible banked pieces.
- Fix: [#7734] Title sequence not included in macOS builds as of 0.2.0 release.
- Fix: [#7756] Steam RCT2 path not correctly checked on macOS and Linux.
- Fix: [#7765] Crash when opening ride list window on Windows Vista.
- Fix: [#7773] Once research has been completed, player is still charged for research.
- Fix: [#7786] Crash when importing a track design.
- Fix: [#7793] Duplicate private keys generated.
- Fix: [#7817] No sprite font glyph for interpunct.
- Fix: [#7823] You can build mazes in pause mode.
- Fix: [#7804] Russian ride descriptions are cut off.
- Fix: [#7872] CJK tooltips are often cut off.
- Fix: [#7895] Import of Mega Park and the RCT1 title music do not work on some RCT1 sources.
- Improved: [#7899] Timestamps in the load/save screen are now displayed using local timezone instead of GMT.
- Improved: [#7918] Better RCT2 detection if both disc and GOG/Steam versions are installed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This issue/PR deserves a changelog entry. network version Network version needs updating - double check before merging!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants