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

cleaning up life cycle code #1068

Merged
merged 5 commits into from
Apr 8, 2021
Merged

cleaning up life cycle code #1068

merged 5 commits into from
Apr 8, 2021

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Apr 7, 2021

related: #767

Intro:

CS recreates all ILoadingExtensions/ISerializableDataExtensions for ALL mods any time ANY mod is hot-swapped. therefore it is a bad idea to rely on a particular instance of such classes. Also, OnCreated/OnRelease are not meaningful.

changes:

  • polished life cycle code a bit.
  • Removed OnCreated() and release().
  • also made everything in lifecycle classes static.
  • this makes the code much less complicated.

Test passed:

start new game -> load saved game -> hot reload -> load saved game -> exit to main manu -> load saved game

@kianzarrin kianzarrin self-assigned this Apr 7, 2021
@kianzarrin kianzarrin marked this pull request as ready for review April 7, 2021 22:19
@kianzarrin kianzarrin added the code cleanup Refactor code, remove old code, improve maintainability label Apr 7, 2021
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.

Looks ok.
I'm not sure going more static instead of less is the right direction, but I don't have a strong opinion in this case. All in all it seems to be an improvement.

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.

Changing ClassName.Instance.Field to be static and using a shorter ClassName.Field is just trading a static (Instance is static) for another static (Field is now static), so there is no change and no step back. But the code using it might indeed look better.

The 🦭 of approval is unsure but rather approves.

@kianzarrin kianzarrin merged commit ebe29ea into master Apr 8, 2021
@kianzarrin kianzarrin deleted the cleanup-loading 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
code cleanup Refactor code, remove old code, improve maintainability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants