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

lifecycle class #1077

Merged
merged 26 commits into from
Apr 12, 2021
Merged

lifecycle class #1077

merged 26 commits into from
Apr 12, 2021

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Apr 10, 2021

fixes #767

  • Introduce a lifecycle class (TMPELifecyckle)
  • the class is a singleton that is created when the TMPE mod is activated and destroyed when the TMPE mod is disabled
  • moved all the static variables in lifecycle code as instant variables to the new class.
  • The TMPELifecycle's finalized is called when TMPE is disabled which proves there is no memory leak :) - I think if anyone uses Singleton<TMPELifecyckle>.instance that would create an extra reference to TMPELifecyckle which prevents the class from finalization leading to a memory leak.
  • moved all the lifecycle related code to the lifecycle namespace (might break NC a bit)

https://ci.appveyor.com/api/projects/krzychu124/tmpe/artifacts/TMPE.zip?branch=life-cycle

depends on PR #1076

@kianzarrin
Copy link
Collaborator Author

I don't know why its like this:
image
I didn't move LoadingExtension.cs to TMPELifeCycle.cs

@kianzarrin kianzarrin self-assigned this Apr 10, 2021
@kianzarrin kianzarrin added the enhancement Improve existing feature label Apr 10, 2021
@kianzarrin kianzarrin linked an issue Apr 10, 2021 that may be closed by this pull request
Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

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

I like this

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

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.

Two issues in code

TLM/TLM/Lifecycle/TrafficManagerMod.cs Show resolved Hide resolved
TLM/TLM/Lifecycle/TMPELifecycle.cs Show resolved Hide resolved
Copy link
Contributor

@DaEgi01 DaEgi01 left a comment

Choose a reason for hiding this comment

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

lgtm besides the points that krzy mentioned.
I m not sure how much is won by the change, since you move global state out of an object that is instantiated by the game and used for lifecycle purposes to global state that lazily instantiates itself and is also meant for lifecycle purposes.
but currently I have no opinion on how to do it better.
the way I normally do that for CSL is also messed up bad.

TLM/TLM/Lifecycle/TrafficManagerMod.cs Show resolved Hide resolved
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.

Looks good! 👍

@kianzarrin kianzarrin added the Dependent This issue is blocked by another issue. label Apr 12, 2021
Base automatically changed from 1074-patcher to master April 12, 2021 10:26
@kianzarrin kianzarrin removed the Dependent This issue is blocked by another issue. label Apr 12, 2021
@kianzarrin kianzarrin merged commit 6b0b6cc into master Apr 12, 2021
@kianzarrin kianzarrin deleted the life-cycle branch April 12, 2021 14:38
@originalfoo originalfoo added this to the 11.6.0 milestone Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lifecycle class
6 participants