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 Only #1448

Merged
merged 2 commits into from
Mar 3, 2022
Merged

API Only #1448

merged 2 commits into from
Mar 3, 2022

Conversation

kianzarrin
Copy link
Collaborator

I created references from TMPE API to TranfficManager Implementations so that other mods will only need to reference TMPE.API.dll

I could not reference TrafficManager from TMPE.API because of the issue of cyclic dependency. So I used reflection instead.

@kianzarrin
Copy link
Collaborator Author

Hot reload is a thing to consider here.

Type.GetType() should resolve to the TrafficManager assembly that was loaded last so I think hotreload will be fine.

@krzychu124
Copy link
Member

Awesome, I also thought about similar thing. Maybe add ///<summary> note that returned value should be cached before use because it's reflection call.

Hot reload is a thing to consider here.
Type.GetType() should resolve to the TrafficManager assembly that was loaded last so I think hotreload will be fine.

For hot reload... They need to be notified somehow that instance changed because static field values are "attached" to assembly - after hot reload if you try reading static field using instance from old assembly you may get garbage data.
I've experienced that problem multiple times when I added TM:PE integration to my experimental mod for spawning vehicles -> after every hot reload of TM:PE I've had to ask about all instances I need once again otherwise most of the things didn't work properly

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Mar 2, 2022

@krzychu124

Awesome, I also thought about similar thing. Maybe add ///<summary> note that returned value should be cached before use because it's reflection call.

Its cached!

static field values are "attached" to assembly

in the old code once TMPE.API is resolved it is also attached to the old assembly so I am not making anythings worse. Improving hot-reload is out of scope of the PR

They need to be notified somehow that instance changed

There is a event for mod state changed. that can be used to reset cache. I already do this in my own mod : https://github.com/kianzarrin/KianCommons/blob/b49f663683804de9d8a9a9349509ee9a26450223/KianCommons/Plugins/AdaptiveRoadsUtil.cs#L12
But this is out of scope of this PR

@kianzarrin kianzarrin self-assigned this Mar 2, 2022
@kianzarrin
Copy link
Collaborator Author

@krzychu124 note that ??= operator calculates only the first time and then uses the cache in subsequent executions.

@krzychu124
Copy link
Member

@krzychu124 note that ??= operator calculates only the first time and then uses the cache in subsequent executions.

Yeah, it looks like I didn't read the code carefully. All clear.

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.

👍

@originalfoo originalfoo added API API for external mods EXTERNAL Mod conflict or other external factor labels Mar 2, 2022
@originalfoo originalfoo added this to the 11.6.5.1 milestone Mar 2, 2022
@kianzarrin kianzarrin merged commit 1fc1e82 into master Mar 3, 2022
@kianzarrin kianzarrin deleted the API-only branch March 3, 2022 10:21
@originalfoo
Copy link
Member

Is this suitable description for the changelog?

[Mod] External mods can now access managers directly via TMPE.API.dll #1448 (kianzarrin)

If not, what would be better text?

@krzychu124
Copy link
Member

Yeah, it's ok IMO

@kianzarrin
Copy link
Collaborator Author

I'd add that referencing TrafficManager.dll is not required anymore.

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Mar 3, 2022

Note:
<ProduceReferenceAssembly>true</ProduceReferenceAssembly> in TLM.csproj generates ref/TrafficManager.dll which only contains the exported symbols and is very small in size:
image

Therefore I don't feel this PR is all that important though, I mean what difference does it make if you reference TMPE.API.dll or refs/TrafficManager.dll. I created this PR anyway for the sake of consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API for external mods EXTERNAL Mod conflict or other external factor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants