Initial Card System Support #330
Conversation
* Currently cards can be equipped/unequipped and synthesized to increase card level. * Loading and Saving Cards works via a Cards Table. * Added Item Properties Table (Save Card Level, Card Exp).
SalmanTKhan
added
the
feature
| /// <param name="idName"></param> | ||
| /// <param name="id"></param> | ||
| /// <param name="properties"></param> | ||
| protected void SaveProperties(string databaseName, string idName, long id, Properties properties, MySqlConnection conn, MySqlTransaction trans) |
There was a problem hiding this comment.
Since the current SaveProperties method already gets its own connection and transaction, we could easily call this from it and reduce the redundant code, right?
| @@ -162,6 +162,8 @@ public Character GetCharacter(long accountId, long characterId) | |||
| } | |||
|
|
|||
| this.LoadCharacterItems(character); | |||
| foreach (var card in this.LoadCharacterItems(character, "cards")) | |||
There was a problem hiding this comment.
For consistency this should probably be its own method, so there's not a loop in the middle of a series of Load calls.
| @@ -423,6 +425,7 @@ public void SaveCharacter(Character character) | |||
| } | |||
|
|
|||
| this.SaveCharacterItems(character); | |||
| this.SaveCharacterItems(character, "cards", character.Inventory.GetCards()); | |||
There was a problem hiding this comment.
Generalized item readers and writers aren't a bad idea, but they might be a bit difficult to handle, because items will usually have some unique values that you can't generalize, like the position for storage items or the status for mail items. Basically, this would turn into something rather complex, or we could just duplicate that little bit of code. I would suggest creating dedicated Load and SaveCards methods.
|
|
||
| if (this.Data.CardGroup != CardGroup.None) | ||
| { | ||
| this.Properties.SetString(PropertyName.CardGroupName, this.Data.CardGroup.ToString()); |
There was a problem hiding this comment.
We should probably mention the importance of the value names in the enum, so nobody gets any ideas about C#-ifying the naming scheme, which would break the property value.
| if (!character.Components.TryGet<TimeActionComponent>(out var timeAction)) | ||
| return DialogTxResult.Fail; | ||
|
|
||
| var timeActionResult = timeAction.StartAsync("ItemCraftProcess", "None", "UPGRADEGEM", TimeSpan.FromSeconds(synthesisTime)).Result; |
There was a problem hiding this comment.
Using Result on the task isn't ideal, because by executing this synchronously, you're putting the thread that is handling this request that came in via the network on pause until the time action returns. Granted, it's only a couple seconds, but we want to handle packets as quickly as possible.
I would recommend using Start and a callback instead, or create an additional async Task method that you call with CallSafe from this one, to more properly implement this asynchronous operation.
| @@ -4474,5 +4484,17 @@ public static void ZC_ACTION_PKS(IActor toActor, IActor fromActor, byte type, in | |||
|
|
|||
| toActor.Map.Broadcast(packet); | |||
| } | |||
|
|
|||
| /// Cancel "timed action" due to mouse movement (walking) | |||
There was a problem hiding this comment.
There seems to be a <summary> missing here. Also, could you clarify what exactly this does? The way you're using it, it doesn't seem related to mouse movement, and it's unclear to me what walking refers to in this context.
| /// <summary> | ||
| /// Gets or sets an expiration date on the item | ||
| /// </summary> | ||
| public DateTime ExpirationDate { get; private set; } = DateTime.MaxValue; |
There was a problem hiding this comment.
This property doesn't appear to get used yet. Did you include it for future use and because of its use in IsExpired, which will never return true in the current code?
There was a problem hiding this comment.
So this was one of those partially cut out of the "card synthesis" code because I'm not sure if we want to go full property on the implementation or not. You can't use an expired material item to upgrade the card. I can remove the expiring item check till we decide what we want to do.
{ namespace: "Item", name: "LifeTime", type: "Number", id: 12614 }, { namespace: "Item", name: "ItemLifeTimeOver", type: "Number", id: 31586 }, { namespace: "Item", name: "ItemLifeTime", type: "String", id: 31587 },
LifeTime defines the duration (in seconds) (or if an item can expire)
ItemLifeTime is the "calculated" date of expiration
ItemLifeTimeOver is a (0 or 1) if the item is expired.
Card Effects aren't implemented yet.