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

Added configurable 'player_save_interval' seconds property #2146

Merged
merged 7 commits into from
Jul 31, 2019

Conversation

dgarson
Copy link
Contributor

@dgarson dgarson commented Jul 21, 2019

Adds a new configuration property for the number of minutes between the automatic save of player characters. This is in response to the the feature request in #1621

@@ -27,11 +27,18 @@ public static class PlayerManager
private static readonly Dictionary<uint, Player> onlinePlayers = new Dictionary<uint, Player>();
private static readonly Dictionary<uint, OfflinePlayer> offlinePlayers = new Dictionary<uint, OfflinePlayer>();

public static readonly long DefaultPlayerSaveInterval = 5;
Copy link
Member

Choose a reason for hiding this comment

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

When I was thinking about this, I was thinking the unit of measurement would be seconds.

Copy link
Contributor Author

@dgarson dgarson Jul 22, 2019

Choose a reason for hiding this comment

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

I'll add a suffix indicating it's seconds.

@@ -47,6 +47,11 @@ public partial class Player : Creature, IPlayer

public float CurrentRadarRange => Location.Indoors ? 25.0f : 75.0f;

public TimeSpan PlayerSaveInterval
Copy link
Member

Choose a reason for hiding this comment

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

This can go back in Player_Database.cs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops. This isn't supposed to be in here anymore. Thanks for catching that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember what happened here. It says PlayerManager is not in scope for Player_Database

// after becoming involved in a PK battle
("player_save_interval", 5) // number of minutes between saving of characters
Copy link
Member

Choose a reason for hiding this comment

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

Seconds make sense here. It's the unit of measurement we use in the majority (all?) of cases in ACE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just using the existing units which was minutes. Seconds works too. I'll switch it over.

@dgarson dgarson changed the title Added configurable 'player_save_interval' minutes property Added configurable 'player_save_interval' seconds property Jul 22, 2019
@Mag-nus
Copy link
Member

Mag-nus commented Jul 22, 2019

Thanks for the seconds change.

My only other thought is that PlayerManager should not be involved.

I think you can just have PlayerSaveInterval in Player_Dabase.cs point to => PropertyManager.GetLong("player_save_interval", DefaultPlayerSaveIntervalSecs).Item;

You can change PlayerSaveInterval to an int so you're not converting every request.

If we want to capture changes from Propertymanager, it should raise an event that also includes the property being changed. Then, objects could subscribe to that event to determine if they need to make an adjustment.

@dgarson
Copy link
Contributor Author

dgarson commented Jul 22, 2019

I found that PropertyManager was also inaccessible from Player_Database either. Any other ideas?

I agree about the PropertyManager change callback but that felt outside the scope of this PR.

@ghost
Copy link

ghost commented Jul 22, 2019

Adding 'using ACE.Server.Managers;' should make PropertyManager "accessible" from Player_Database.cs. As far as I can see, the relevant API functions of PropertyManager are all set to public.

@dgarson
Copy link
Contributor Author

dgarson commented Jul 26, 2019

@Mag-nus any more changes I should make to this PR before it's ready to merge?

Copy link
Member

@Mag-nus Mag-nus 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 test this to confirm the timings are working as intended, but, it looks good.

@gmriggs gmriggs merged commit eeaa604 into ACEmulator:master Jul 31, 2019
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