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

Api update #1577

Merged
merged 15 commits into from
May 29, 2022
Merged

Api update #1577

merged 15 commits into from
May 29, 2022

Conversation

kianzarrin
Copy link
Collaborator

added API so that node controller access TMPE through API.
node controller is still patching Junction restriction manager. that I can fix in next PR.
also added not implemented pedestrian/bike lane transition groups for better future proofing of AN.

Tested with:
AN-alpha
HUT-DCR.zip
Hide crossings

@kianzarrin kianzarrin added the API API for external mods label May 22, 2022
@kianzarrin kianzarrin self-assigned this May 22, 2022
@kianzarrin kianzarrin marked this pull request as ready for review May 22, 2022 21:04
Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

Code looks OK, not tested in-game though.

Copy link
Contributor

@Elesbaan70 Elesbaan70 left a comment

Choose a reason for hiding this comment

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

I didn't dive too deeply into implementation, but focused on the API instead. Implementation can always be changed. API, we're stuck with.

TLM/TLM/Constants.cs Outdated Show resolved Hide resolved
TLM/TMPE.API/Manager/ITrafficLightManager.cs Outdated Show resolved Hide resolved
TLM/TMPE.API/UI/IRoadSignTheme.cs Outdated Show resolved Hide resolved
@kianzarrin kianzarrin requested a review from kvakvs May 24, 2022 20:49
@kianzarrin
Copy link
Collaborator Author

@kvakvs @Elesbaan70 I made changes to UI API. Do you like my new ITheme and IUIFactory?

@kvakvs
Copy link
Collaborator

kvakvs commented May 24, 2022

@kvakvs @Elesbaan70 I made changes to UI API. Do you like my new ITheme and IUIFactory?

Nice, keep it simple. One point access to all future UI interfaces.

Copy link
Contributor

@Elesbaan70 Elesbaan70 left a comment

Choose a reason for hiding this comment

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

These changes are mostly good, but the renaming of JunctionRestrictionFlags and its members needs to be rolled back, and CanToggleTL needs to be renamed to CanToggleTrafficLight and privately implemented to protect compatibility.

TLM/TMPE.API/Traffic/Enums/JunctionRestrictionRules.cs Outdated Show resolved Hide resolved
@@ -0,0 +1,9 @@
namespace TrafficManager.API.Traffic.Enums {
public enum TrafficLightType {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think TrafficLightType is the right name for this. It includes things like "paused."

It's pretty UI-centric. Maybe its name should reflect that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what should I call it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TrafficLightFlags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Paused traffic light acts like normal vanilla traffic light.
I also have the option of dropping paused.

Copy link
Contributor

Choose a reason for hiding this comment

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

I consider this to be a specifically UI-related enumeration. Basically, it indicates which traffic light tool has control of the light, and indicates additional state information. So why not call it TrafficLightToolState, and then ITrafficLightManager.GetTrafficLightToolState and ITheme.TrafficLightToolState()?

Copy link
Contributor

Choose a reason for hiding this comment

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

And no, I say don't drop paused. Under my interpretation, it belongs there.

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

Minor suggestions for consideration, nothing blocking


Texture2D VehicleRestriction(ExtVehicleType type, bool allow);

Texture2D GetTrafficLightIcon(ushort nodeId);
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
Texture2D GetTrafficLightIcon(ushort nodeId);
Texture2D TrafficLightIcon(ushort nodeId);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

must be public though.

/// <summary>
/// returns if the node as any kind of traffic light (vanilla, manual, timed).
/// </summary>
public bool HasTrafficLight(ushort nodeId);
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
public bool HasTrafficLight(ushort nodeId);
bool HasTrafficLight(ushort nodeId);

implicitly public so redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@krzychu124 no that does not work
image
Interface Implementation has to be public.

unless if I do like this:
image

But that is not necessary in this case.

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 might not be a bad idea to implicitly define all API implementations that we do not want to use internally. If we want to go that route we need to have a discussion about it and apply the rule consistently everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

you get an error probably because you are explicitly implementing the interface and implementation you shown is private then making it explicit you force us to cast to the interface to use that method (is there another one with the same signature?). Explicit and implicit implementation are completely different things with some implications.
Basically when you create explicit implementation you won't be able to use it like this
TraffLightManager.Instance.CanToggleTrafficLight() (assuming that Instance returns TrafficLightManager, not the interface)
but forced to do this instead:
((ITrafficLightManager)TraffLightManager.Instance).CanToggleTrafficLight()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What you said first does not work:

Suggested change
public bool HasTrafficLight(ushort nodeId);
bool HasTrafficLight(ushort nodeId);

but this will work:

Suggested change
public bool HasTrafficLight(ushort nodeId);
bool ITrafficLightManager.HasTrafficLight(ushort nodeId);

I understand how implicit/explicit works.

Copy link
Contributor

@Elesbaan70 Elesbaan70 left a comment

Choose a reason for hiding this comment

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

I second the last couple of recommendations from @krzychu124, for naming consistency and clean-looking code. Otherwise looks good.

@kianzarrin
Copy link
Collaborator Author

Cool. I will update NC/HUT/DCR/HTC (AN already uses TMPE API) after both stable and Test are using this Hotfix

@kianzarrin kianzarrin merged commit 2a4c104 into master May 29, 2022
@kianzarrin kianzarrin deleted the API-update branch May 29, 2022 10:19
krzychu124 pushed a commit that referenced this pull request May 31, 2022
API update for compatibility with NC/DCR/HUT
* TrafficLights/JunctionRestrictions: updated API
* LaneEndTransitionGroup : Added Bicycle and Pedestrian for future proofing.
* Expose Road sign theme in API.

(cherry picked from commit 2a4c104)
@krzychu124 krzychu124 mentioned this pull request May 31, 2022
8 tasks
kianzarrin added a commit that referenced this pull request Jun 1, 2022
API update for compatibility with NC/DCR/HUT
* TrafficLights/JunctionRestrictions: updated API
* LaneEndTransitionGroup : Added Bicycle and Pedestrian for future proofing.
* Expose Road sign theme in API.
kianzarrin added a commit that referenced this pull request Jun 1, 2022
API update for compatibility with NC/DCR/HUT
* TrafficLights/JunctionRestrictions: updated API
* LaneEndTransitionGroup : Added Bicycle and Pedestrian for future proofing.
* Expose Road sign theme in API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API for external mods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants