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

fix: Refactor GoGump, moves LocationTree to GoLocations #1804

Merged
merged 8 commits into from
Jun 9, 2024

Conversation

nerun
Copy link
Contributor

@nerun nerun commented May 30, 2024

This update is not mine, I don't know the author. I got this from Discord, but it makes perfect sense to me. In the author words:

Overview

The GoGump class was updated to comply with the constraints of static readonly fields in C#. Specifically, static readonly fields cannot be assigned to outside of a static constructor or a variable initializer. The changes ensure that these fields are properly initialized.

Detailed Changes

Added Static Constructor:

A static constructor was introduced to handle the initialization of static readonly fields.

Static constructors are special constructors that are called automatically before any static members are accessed and are ideal for initializing static fields.

Initialization of Static Readonly Fields:

The fields Felucca, Trammel, Ilshenar, Malas, Tokuno, and TerMur are static readonly. These fields represent different LocationTree instances.

Previously, these fields were being assigned values within the DisplayTo method, which caused compilation errors because static readonly fields can only be assigned in a static constructor or variable initializer.

The assignments were moved to the static constructor to comply with the language rules.

Removed In-Method Assignments:

Assignments within the DisplayTo method were removed because they were moved to the static constructor.

The DisplayTo method now simply checks the current map and sets the appropriate LocationTree without attempting to assign values to the static readonly fields.

Additionally, this update also allows the famous Staff Runebook script to work.

@Voxpire
Copy link
Collaborator

Voxpire commented May 30, 2024

I would recommend implementing these fields as properties using the read only auto pattern;
public static object Object { get; }
The original private fields were being lazy loaded, so the least intrusive change to the system would be to use a private setter;
public static object Object { get; private set; }
But wouldn’t have resolved issues with data being unavailable, which you resolved with the static ctor.

@kamronbatman
Copy link
Contributor

TODO

We want to make this, and other map related functionality more data driven so that customizing your maps doesn't require gutting code in several places.

@kamronbatman
Copy link
Contributor

@nerun - One last change. If the LocationTree is used outside of this script, maybe we should move the logic for determining which tree to it's own function. Something like GetLocationTree(Map map)?

This brings me to the next proposed change (optional for this PR). Should we move the LocationTree concept to the map directly, so it is defined outside of a Gump?

This way the API would be:

map.LocationTree

Instead of the if/else-if to find the right one referenced by static constants.

Thoughts?

@kamronbatman kamronbatman changed the title GoGump code optimization fix: Refactor GoGump and make LocationTree public. May 31, 2024
@kamronbatman kamronbatman changed the title fix: Refactor GoGump and make LocationTree public. fix: Refactor GoGump, moves LocationTree to GoLocations Jun 9, 2024
@kamronbatman kamronbatman merged commit 9847efb into modernuo:main Jun 9, 2024
11 of 12 checks passed
@nerun nerun deleted the gogump branch June 9, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants