-
Notifications
You must be signed in to change notification settings - Fork 241
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
Saving Character to Database #143
Saving Character to Database #143
Conversation
Characters now level up behind the scenes Gave the ability to level up with effects and message Added Network Message Type for Ints Removed debugging and updated comments Added GameMessagePrivateUpdatePropertyInt Allow for max level and lock Changed to a long to allow greater amounts of XP
Added level up skill credit text and max level Added skill credits and comments Removed my comments and started using queries to get data from the chart
Fixed up the grantxp command. Now limited to 999,999,999,999. Fixed the one-line if statement causing headaches; please test! Added final level effect, moved effect after the network message to perhaps prevent an order issue. Now limits the grantxp ammount, until you're max level. Added a way to add xp after max level Now locks total xp properperly on max level
Removed a debug test Unset networktrace Readded a line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could get these things fixed soon, that'd be great. If not, let me know and we can get them merged as is and you can update it with the changes later.
@@ -151,12 +171,18 @@ public bool IsNameAvailable(string name) | |||
return (charsWithName == 0); | |||
} | |||
|
|||
public Task UpdateCharacter(Character character) | |||
public async void UpdateCharacter(Character character) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thou shalt not async void.
command.Parameters.Add("", query.Item1.Types[i]).Value = query.Item2[i]; | ||
|
||
#if NETWORKDEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use log4net instead, please.
|
||
message = $"Unable to find a effect called {parameters[0]} to play."; | ||
|
||
if (Enum.TryParse(parameters[0], true, out effect)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on non-1-liner "if" statements, we really need to use explicit "{" and "}" stuff. took me a while to visually parse this.
//ex...more info.. if needed.. | ||
ChatPacket.SendServerMessage(session, $"Invalid Effect value", ChatMessageType.Broadcast); | ||
return; | ||
//ChatPacket.SendServerMessage(session, parameters[0], ChatMessageType.Broadcast); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to do something in the catch, even if it's a comment that it's intentionally blank/empty.
var effectevent = new GameMessageEffect(this.Guid, effectid); | ||
Session.Network.EnqueueSend(effectevent); | ||
var effectEvent = new GameMessageEffect(this.Guid, effectId); | ||
Session.Network.EnqueueSend(effectEvent); | ||
} | ||
|
||
/// <summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In SpendSkillXp below, you're not taking usage-earned xp into account:
uint currentXp = chart.Ranks[Convert.ToInt32(skill.Ranks)].TotalXp;
You don't need to fix it, but you need to document it in a TODO or a known issue.
Source/ACE/Entity/Player.cs
Outdated
} | ||
catch (NullReferenceException) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either comment why this is empty, handle the exception, or remove the catch.
Source/ACE/Entity/Player.cs
Outdated
} | ||
catch (NullReferenceException) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either comment why this is empty, handle the exception, or remove the catch.
Source/ACE/Entity/Player.cs
Outdated
@@ -728,9 +894,9 @@ public void Logout(bool clientSessionTerminatedAbruptly = false) | |||
|
|||
// NOTE: Adding this here for now because some chracter options do not trigger the GameActionSetCharacterOptions packet to fire when apply is clicked (which is where we are currently saving to the db). | |||
// Once we get a CharacterSave method, we might consider removing this and putting it in that method instead. | |||
DatabaseManager.Character.SaveCharacterOptions(character); | |||
//DatabaseManager.Character.SaveCharacterOptions(character); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You commented this out, but why? If it's implemented elsewhere, just delete it. if not, it'd be good to know why this is no longer needed.
|
||
public override void Handle() | ||
{ | ||
Position marketplaceDrop = new Position(23855548, 49.16f, -31.62f, 0.10f, 0f, 0f, -0.71f, 0.71f); // Is this the right drop? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convert this into a static readonly position object, please. no need to allocate a new one every time - it just creates work for the garbage collector.
"Ripley has been saved."
This PR includes commits from the following PRs:
#117
#119
#125
#134
#141
This PR addresses issues:
#51
#52
#53
Code work was done by @fantoms and myself
Accepting this PR should mean closing without merging the previously mentioned PRs and mostly resolves the issues.
Spending Skill Credits is not yet addressed.