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

separete lane connections for track/road vehicles. #1550

Merged
merged 42 commits into from
Jun 12, 2022
Merged

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Apr 26, 2022

fixes #354

  • hotkeys and icons from [EPIC] UI design of Lane connector tool for new features. #1479
  • trolleybuses use same connections as cars.
  • used Road/Track sub managers to store lane connections.
  • all other data structures need to use separate arrays to store road/track connections.
  • lane connection manager does not allow making unsupported lane connection of the wrong lane/vehicle type. this helps with backward compatibility too. but it does allow making backward connections when not allowed for the sake of backward compatibility on train stations. this should change in set dead ends for lanes using lane connector #1213.

Bonus:

TMPE.zip

pictures are in the comments.

Tests:

  • car only test: always in car mode. holding alt/shift does not change mode
  • track only test: always in track mode. holding alt.shift does not change mode
  • other node test: regardless of whether or not current node is car-only or track-only none-selected nodes show all car+track connections. if alt is held then they only show track connections.
  • tram road test: holding alt make tram roads select-able. otherwise only car lanes are select-able.
  • mixed lane mode: normally you see circle and can make car connection. holding alt shows triangle and can make track connection. holding shift can make both connections.
  • bidirectional lane: node marker is diamond and you can make forward/backward connection. lane curves have arrows on them indicating their direction.
  • acute+mixed test: hold shift to select mixed lanes. acute targets should show circle, and clicking them should only make car lane connection
  • acute only mixed lane test: connect a road with mixed tram lane to an intersection with no other tram rods. expected: no
    target lane connection is highlighted. shift clicking will put in car mode.
  • source to target test: make sure you can only make lane connection in the right direction.
  • hintbox text: go through different state and check if hintbox is OK. make sure ALT/SHIFT modifiers are only there when selecting car+tram node.
  • big roundabout test: make sure roundabout connector makes stay in lane is applied on entry and inside of the roundabout when there is a node too close to the roundabout and when there are nodes inside roundabout. related StayInLane gets confused with multi pupose car/track lanes. #705
  • mixed lane arrow test: connect trams only on a mixed lane and make sure you can still modify lane arrows. make sure that you can't modify lane arrows if you connect cars only.
  • separate turning lane: in above scenario make sure separate turning lanes is blocked iff there is a car connection.

mixed connections tests: connect car and track differently on a mixed car+tram lane and:

  • mixed lane routing test: verify the are routed differently.
  • refresh test: exit and reenter tool, select the node and make sure you can see all connections.
  • lane arrow test: lane arrows are influenced only by car connection.
  • mixed lane persistency test: make sure they connections persist when save/load game
  • mixed lane record test: make sure connections can be copied by move it.

backward compatibility test:

  • save game backward compatibility test: connect mixed lanes in old TMPE and saved it. make sure they get both car/track connection when loaded with this version.
  • lane record backward compatibility test: connect mixed lanes in old TMPE and export it. make sure they get both car/track connection when imported with this version.

TODO:

  • on mixed lanes do not allow trams to make acute lane connections.
  • hints

@kianzarrin kianzarrin added UI User interface updates DO NOT MERGE YET Don't merge this PR, even if approved, until further notice LANE ROUTING Feature: Lane arrows / connectors labels Apr 26, 2022
@kianzarrin kianzarrin added this to the 11.6.5.3 milestone Apr 26, 2022
@kianzarrin kianzarrin self-assigned this Apr 26, 2022
@kianzarrin kianzarrin marked this pull request as ready for review April 27, 2022 00:02
don't show track node marker when acute
fixed mixed acute only.
@kianzarrin kianzarrin marked this pull request as draft April 27, 2022 08:33
@kianzarrin kianzarrin linked an issue Apr 27, 2022 that may be closed by this pull request
4 tasks
kianzarrin added 2 commits May 8, 2022 15:19
polished overlay code: use `DrawShape` to draw everything.
@kianzarrin
Copy link
Collaborator Author

kianzarrin commented May 8, 2022

I ditched triangle and am using square now. Triangle looks a bit ugly and is hard to write the code.
image

Also the diamond is no longer filled:
image

@originalfoo
Copy link
Member

Looks good!

@kianzarrin
Copy link
Collaborator Author

about backward compatibility at train stations:
In TMPE stable one lane connection like bellow is enough to make trains stay in lane.
image
The same is true with TMPE in this PR:
image

This makes backward compatibility easy. There is no need to keep connections that from target to source when loading legacy game.

Removing connections in the wrong direction also solves this issue that happens when you remove lane connections that was made using legacy TMPE:
image

@kianzarrin kianzarrin requested a review from Elesbaan70 May 11, 2022 23:58
originalfoo added a commit that referenced this pull request May 22, 2022
- [Meta] This release adds a new language, updates translations, and improves lane routing
- [New] Veitnamese translation #1551 (DucAnhLam)
- [Updated] Separate road and track lane routing #1550 #1546 #1545 #354 (kianzarrin)
- [Updated] Translations for multiple languages #1551 #1344 (Nguyễn Tài Đức, OldEj, AduitSSH, Chamëleon, 문주원, krzychu124, shg166, John Lok Ho, Márcio Saeger, DucAnhLam, DNSE, Arne Peirs, Neoone, Fatih YILDIRIM, Zeldslayer, GiorgioHerbie, krzychu124)
@krzychu124
Copy link
Member

Sync with master :)

@@ -186,12 +187,15 @@ public class LaneConnection : ISerializable {

public bool sourceStartNode;

public bool Legacy => SerializableDataExtension.Version < 2;
public LaneEndTransitionGroup group = LaneEndTransitionGroup.Vehicle;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am concerned about continuing to introduce breaking file format changes. Can this all wait until the new persistence code is merged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR has waited for a month already. I don't think we need to wait more. conflicts will accumulate over time.
Is it breaking anything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it does not break anything for me. I made the connections bellow with TMPE from this PR then saved and loaded the game using stable TMPE:
image

Copy link
Collaborator Author

@kianzarrin kianzarrin Jun 9, 2022

Choose a reason for hiding this comment

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

If we don't want to merge it with master, we should consider merging a feature branch instead of stopping this PR. but it is forward/backward compatible so I don't think this is good to go to master.

Copy link
Member

@krzychu124 krzychu124 Jun 9, 2022

Choose a reason for hiding this comment

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

It's exactly the same situation as last time. If you save game with this build you won't be able to load it using current master or WS stable

Copy link
Member

@krzychu124 krzychu124 Jun 9, 2022

Choose a reason for hiding this comment

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

I think it doesn't matter because the change in any of serializable configuration classes will make the savegame incompatible with previous version of deserializer (current master or any workshop version)

Copy link
Contributor

Choose a reason for hiding this comment

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

You sure about that? I encountered the problem specifically in relation to adding a new type, which is still a pretty drastic limitation when doing work of any significance, since the whole system is type-based. #1559 explains it as an issue with strongly typed serialization.

Copy link
Member

Choose a reason for hiding this comment

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

ok, did not throw exception or anything, loaded something, looks good. Ignore previous comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

loaded something

it is both forward/backward compatible.
if you make different lane connections on mixed/car track lanes -> save -> load with earlier version of TMPE, then car/track lane connections will merge.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, noticed that, looks ok.

Copy link
Contributor

@chameleon-tbn chameleon-tbn left a comment

Choose a reason for hiding this comment

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

lgtm

@kianzarrin kianzarrin merged commit f709191 into master Jun 12, 2022
@kianzarrin kianzarrin deleted the 354-phase3 branch June 12, 2022 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE YET Don't merge this PR, even if approved, until further notice LANE ROUTING Feature: Lane arrows / connectors UI User interface updates
Projects
None yet
6 participants