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
Dismemberment of storage_data
from mmo_charstatus
.
#1722
Conversation
beaf064
to
2e06aab
Compare
2e06aab
to
9eb5d3e
Compare
see travis log. look like wrong sign in format strings |
9eb5d3e
to
e9cba36
Compare
Irrelevant as of https://travis-ci.org/HerculesWS/Hercules/builds/226807632 |
@@ -8032,29 +8032,29 @@ int HP_inter_quest_parse_frommap(int fd) { | |||
return retVal___; | |||
} | |||
/* inter_storage_interface */ | |||
int HP_inter_storage_tosql(int account_id, struct storage_data *p) { | |||
int HP_inter_storage_tosql(int account_id, struct storage_data *cp, struct storage_data *p) { |
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.
probably better split auto generated HPM files into separate commit
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.
Mostly reviewed the char/inter server part (only glanced at the map server part, I'll review it more thoroughly on the next iteration.
Good job on the saving algorithm so far. It was tough to find something to nitpick!
src/char/int_storage.c
Outdated
struct item *cp_it = &VECTOR_INDEX(cp->item, i); | ||
struct item *p_it = NULL; | ||
|
||
ARR_FIND(0, VECTOR_LENGTH(p->item), j, |
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.
Can you also add a check to skip the items that have their updated_p[j]
flag already set? (it might happen in some edge cases, i.e. identical items that can't be stacked for any reason but don't have an unique id) It would make the saving more robust
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.
Oops, this slipped my mind. Added.
src/char/int_storage.c
Outdated
*/ | ||
VECTOR_CLEAR(cp->item); | ||
VECTOR_ENSURE(cp->item, VECTOR_LENGTH(p->item), 1); | ||
VECTOR_PUSHARRAY(cp->item, VECTOR_DATA(p->item), VECTOR_LENGTH(p->item)); |
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.
This can't be done, unfortunately. The id
field in p->item
isn't safe to rely on (it contains garbage -- or at least values not relevant to the storage
table -- if the items were moved from/to an inventory or a cart). You need to clear cp
and then query it again (unless you want to update it piece by piece in this function while doing each REPLACE/INSERT/DELETE query)
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.
I think querying the table for a final sync would be better because there is no proper way to get the id
field of the table after insertion (without splitting the insert queries per item), so I'll go with that.
|
||
VECTOR_INIT(p_stor.item); | ||
|
||
if (count > 0) { |
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.
Doesn't this also need to run if count is 0? (storage that used to contain items and has just been emptied by the owner)
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.
I went ahead and added more than the required number of lines into the clause. Fixed and added.
src/char/int_storage.c
Outdated
return 1; | ||
} | ||
// storage data finalize | ||
void inter_storage_sql_final(void) | ||
{ | ||
inter_storage->account_storage->destroy(inter_storage->account_storage, NULL); |
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.
This should call a destructor function that calls VECTOR_CLEAR() on each remaining entry before it gets autoreleased
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.
Oops, this one escaped me as well. Thank you. Added.
src/common/mmo.h
Outdated
int storage_amount; | ||
struct item items[MAX_STORAGE]; | ||
bool save; //< save flag. | ||
uint32 aggregate; //< total item count. |
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.
Can you make this a signed variable instead? I don't really want to see any unsigned used for any reason other than bitmasks or shifts (unless it's something that the client forces us to do)
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.
Okay, fixed.
e9cba36
to
81e662b
Compare
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.
Reviewed the map server part as well this time.
There's another thing I didn't mention in the line comments, but I'd like to point out here: there can be some race conditions that wold cause very nasty issues. The itemcheck that occurs on login, should never happen before the storage has been received for example.
A possible solution:
sd->state.itemcheck
becomes a bitmask (with separate flags for the storage and the inventory- pc_checkitem skips the storage check if it's called when the storage hasn't been received yet.
- pc_checkitem is also called when the storage is received (this means that it can be called twice, but since the itemcheck flag is a bitmask, it'll only check the parts that haven't been checked each time)
- the functions that open the storage (including atcommands, script commands) fail if the storage hasn't been received yet
If you have other solutions, they're of course welcome, but I think this is a reasonably simple approach
src/char/int_storage.c
Outdated
StrBuf->Clear(&buf); | ||
StrBuf->Printf(&buf, "DELETE FROM `%s` WHERE `id` IN (", storage_db); | ||
for (i = 0; i < total_deletes; i++) { | ||
StrBuf->Printf(&buf, ", '%d'", delete[i]); |
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.
This adds an extra comma at the beginning: DELETE FROM
%sWHERE
id IN (, '%d', '%d' [...]);
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.
Fixed, comma will not be added on the first iteration.
src/char/int_storage.c
Outdated
StrBuf->Printf(&buf, ", `card%d`", k); | ||
for (k = 0; k < MAX_ITEM_OPTIONS; k++) | ||
StrBuf->Printf(&buf, ", `opt_idx%d`, `opt_val%d`", k, k); | ||
StrBuf->Printf(&buf, ", `expire_time`, `bound`, `unique_id`) VALUES"); |
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.
There are a few Printf that can be changed to AppendStr
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.
Changed to AppendStr.
src/char/int_storage.c
Outdated
for (i = 0; i < total_deletes; i++) { | ||
StrBuf->Printf(&buf, ", '%d'", delete[i]); | ||
} | ||
StrBuf->Printf(&buf, ");"); |
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.
Another AppendStr candidate
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.
Changed to AppendStr.
src/char/int_storage.c
Outdated
StrBuf->Printf(&buf, ", '%d'", p_it->card[j]); | ||
for (j = 0; j < MAX_ITEM_OPTIONS; ++j) | ||
StrBuf->Printf(&buf, ", '%d', '%d'", p_it->option[j].index, p_it->option[j].value); | ||
StrBuf->Printf(&buf, ")%s", total_inserts > 0 ? ", " : ""); |
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.
Umm, the comma is misplaced here, it looks like it should be part of the Printf above (the one that opens the (
).
This one can the be changed to AppendStr.
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.
Uhm, same thing no? (output-wise)
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.
Umm, no, the comma you need to control is the one at the beginning of the line, not the one at the end:
INSERT INTO `storage` ( ... ) VALUES -- beginning, only printed once
( ... ) -- this one lacks a comma
( ... ),
( ... ),
( ... ),
( ... ), -- there's an extra comma here
vs.
INSERT INTO `storage` ( ... ) VALUES
( ... ) -- no comma
, ( ... )
, ( ... )
, ( ... )
, ( ... )
(the line endings are added just for the example purpose, the query is all in one 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.
Ah, of course. 🙄
src/map/intif.c
Outdated
|
||
WFIFOHEAD(inter_fd, len); | ||
WFIFOW(inter_fd, 0) = 0x3011; | ||
WFIFOW(inter_fd, 2) = (uint16) len; |
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.
The actual sent length can be reduced to 8 + total * sizeof (struct item)
(you can call WFIFOHEAD with the current value, then update WFIFOW(inter_fd , 2)
at the end of the for(), and call WFIFOSET() with the updated length. This would avoid sending garbage/empty data at the end of the packet.
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.
Since there is no need of sending zeroed entries, we can cache the available items before sending a copy. (This should be the proper way)
src/map/storage.c
Outdated
it = &VECTOR_INDEX(sd->storage.item, i); | ||
} | ||
|
||
*it = *item_data; |
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.
This line is redundant if the if
condition above is true. How I'd write it:
if (i == VECTOR_LENGTH(sd->storage.item)) {
VECTOR_ENSURE(sd->storage.item, 1, 1);
VECTOR_PUSH(sd->storage.item, *item_data);
it = &VECTOR_LAST(sd->storage.item);
} else {
it = &VECTOR_INDEX(sd->storage.item, i);
*it = *item_data;
}
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.
Yes, thanks, has been fixed.
src/map/storage.c
Outdated
clif->updatestorageamount(sd, sd->status.storage.storage_amount, MAX_STORAGE); | ||
Assert_retr(1, n >= 0 && n < VECTOR_LENGTH(sd->storage.item)); | ||
|
||
nullpo_retr(1, (it = &VECTOR_INDEX(sd->storage.item, n))); |
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.
no need to use nullpo_retr()
here, VECTOR_INDEX()
can't return NULL
(well, it theoretically can, but only for n == 0 && VECTOR_DATA(foo) == NULL
, but that can't be the case)
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.
Makes sense, removed the nullpo check here.
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.
Along with the assignment you had hidden inside the nullpo ;)
Please restore the assignment, or it
will be NULL for the rest of the function
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.
🤦♂️
src/map/storage.c
Outdated
|
||
Assert_retr(1, amount <= it->amount); | ||
|
||
Assert_retr(1, it->nameid > 0); |
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.
There are places where this might get called even if it->nameid
is 0 (example: @clearstorage
). It's fine to keep the assertion here, but if that's the case, you need to make sure it's not 0 on the caller's side. Else change this assertion to an if
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.
There were checks everywhere else except @clearstorage
, and I don't see the point in it parsing a zeroed entry so I've added checks on the caller's side. I think since we're using vectors, and we'd have to iterate through the elements to pass them into the function, the caller could also perform an additional check for zeroed entries? (Or if just silently returned by this function it would prevent coding additional lines for the check from every where that function needs to be called.) So what would be right in this scenario?
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.
Yup, adding the checks on the caller's side sounds good. There's no point in calling this function to delete an item that doesn't exist to begin with
src/map/storage.c
Outdated
|
||
sd->storage.save = true; | ||
|
||
clif->storageitemremoved(sd, n, amount); |
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 the case of @clearstorage
or the itemcheck, you shouldn't send this packet (that's the reason for the STORAGE_FLAG_NORMAL
check in the original code: if the client isn't showing the storage, we shouldn't send an update packet)
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.
I think I removed this because I had earlier added the check at the beginning of the function. Re-added because it was removed.
src/map/storage.c
Outdated
nullpo_retv(sd); | ||
|
||
Assert_retv(sd->state.storage_flag == STORAGE_FLAG_NORMAL); |
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.
There are some cases that call this function just to ensure that the storage is saved, even if it's not open in the client (item check, @clearstorage
). This assertion would fail
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.
Yes, not sure how that bit was designed to work so I'm going to remove this line here.
@MishimaHaruna I will add |
@Smokexyz looks good, only the two points I commented on are still open (other than itemcheck and HPM) |
you might want to squash your commits |
9999de5
to
340fe8d
Compare
@MishimaHaruna Squashed and updated. Changes are mentioned in the commit message. |
|
||
if (!id) | ||
continue; | ||
if (sd->itemcheck & PCCHECKITEM_INVENTORY) { |
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.
After each check is complete, you should unset the corresponding flag from sd->itemcheck
(i.e. sd->itemcheck &= ~PCCHECKITEM_FOO
)
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.
Is this necessary? It is not really treated as a state in the code. There are no checks for whether the player is in the state either. What if the function took another parameter that indicates the type of check?
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.
Umm, the point of this flag is so that the check doesn't get unnecessarily repeated (it was the same before this pull request as well, it'd unset the flag once the check was completed). After this pull request it becomes more important to flag it, so that you can safely call the function twice, without repeating the check unnecessarily (and wastefully): once when the character logs in, and the second time when the storage is initially received.
if (sd->vd.body_style) | ||
clif->refreshlook(&sd->bl,sd->bl.id,LOOK_BODY2,sd->vd.body_style,SELF); | ||
// item | ||
clif->inventorylist(sd); // inventory list first, otherwise deleted items in pc->checkitem show up as 'unknown item' |
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.
I think this comment is relevant to keep
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.
This would be obvious because pc_delitem
in pc_checkitem
sends clif_delitem
, which only makes sense if the client has received the inventory list before asking it to delete something. But okay, I'll put it back.
src/map/clif.c
Outdated
@@ -9338,7 +9341,7 @@ void clif_parse_LoadEndAck(int fd, struct map_session_data *sd) { | |||
sd->state.warping = 0; | |||
sd->state.dialog = 0;/* reset when warping, client dialog will go missing */ | |||
|
|||
// look | |||
//< Character Looks |
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.
There's an extra <
sign here. Please note the syntax recognized by doxygen:
int foo; ///< Description of foo (three slashes and a left arrow to point to the variable declaration to the left)
or
/// Description of foo (three slashes and no arrow to refer to the following declaration)
int foo;
or
/**
* Description of foo
* (c-style block comment starting with slash star star, also refers to the following declaration)
*/
int foo;
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.
But if it recognizes //
, and it should, anything after it would be treated as a comment no?
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.
No, the //
(normal comment) is ignored, only ///
(one extra slash) is recognized by doxygen. And ///<
has the special meaning I described.
Now that I re-read my message, it looks like I encouraged to put a doxygen comment here, but that wasn't my intent (tiredness plays weird tricks, sorry). What I meant to say here is that the notation you used, would make it look like an attempt to insert a doxygen comment, while it isn't. You can just remove the extra <
and have a normal comment here (it's not documenting a variable after all)
src/map/intif.c
Outdated
|
||
sd->storage.received = true; //< Mark the storage state as received. | ||
sd->storage.save = false; //< Initialize the save flag as false. | ||
} |
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.
This seems a good place to re-attempt a second itemcheck, in case the storage wasn't available yet at the time the first itemcheck runs (this time it'll only run the storage part, since the other flags have been unset already)
src/map/pc.c
Outdated
|
||
storage->delitem(sd, i, it->amount); | ||
|
||
storage->close(sd); |
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 need to defer this storage->close()
to the end of the loop (and only if at least an item was deleted).
storage->close()
will compact the array, so if you do it during the loop, the index will be invalidated.
340fe8d
to
bc6a774
Compare
@MishimaHaruna Updated with changes. Sorry for the delay, I've been busy. |
I receive this error while trying to store 1 blue potion using @storeall. Im sure that my sql is updated.
|
src/map/intif.c
Outdated
|
||
if (sd->storage.save == false) | ||
// Assert that at this point in the code, both flags are true. | ||
Assert_retv(sd->storage.save == true); |
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.
I receive an error after testing this.
(04/13/2017 05:06:24) [ Error ] : --- failed assertion --------------------------------------------
(04/13/2017 05:06:24) [ Error ] : c:\users\wedevgames\desktop\ro test servers\herc_04252017\src\map\intif.c:524: 'sd->storage.save == 1' in function `unknown'
(04/13/2017 05:06:24) [ Error ] : --- end failed assertion ----------------------------------------
@officialwedevgames Thanks for the testing. Updated with fixes. |
7cfda45
to
5996128
Compare
Reviewed 5 of 17 files at r1, 1 of 3 files at r3, 1 of 6 files at r4, 3 of 12 files at r5, 6 of 8 files at r6, 4 of 4 files at r7, 7 of 7 files at r8. src/map/intif.c, line 506 at r7 (raw file):
No need to reset this flag, or it'll run all the checks again. Just let it run the remaining checks only, this line can be removed src/map/intif.c, line 539 at r7 (raw file):
This is still leaving holes in the sent data. Why not just use two index variables? Loop with Comments from Reviewable |
Remove loading and saving of storage_data through char.c Re-declaration of structure storage_data as a vector. Re-code of portions in the map-server using storage_data. A new approach is taken by saving the loaded storage data from sql into memory for the duration of the session, thereby removing the need of querying the database to re-load all items everytime a storage save routine is issued from the map-server. Saving of storage items is done through a new function that significantly reduces the number of queries compared to char_memitemdata_tosql(), and therefore run-time speed. This method potentially reduces the number of update and delete queries from MAX_STORAGE (which could be >= 600) times to literally 1. Storage items are stored in a dynamically allocated array and handled accordingly. struct mmo_charstatus size reduces by 34,800 bytes. Update pc_checkitem() with masks for item checks. `sd->state.itemcheck` has been changed to `sd->itemcheck` of type `enum pc_checkitem_types` `battle/items.conf` has been updated to reflect configuration changes. Further updates to assert a successful reception of storage data in related functions.
5996128
to
e8affc4
Compare
@MishimaHaruna Fixed. |
I believe this is ni a good state to merge now. Pinging @4144 to mark the changes as approved if it looks good |
Reviewed 8 of 8 files at r9, 7 of 7 files at r10. Comments from Reviewable |
I'm merging this now. Any further changes will be made as follow-up commits. Thank you very much to @Smokexyz and the reviewers. This was a pretty big one. |
Pull Request Prelude
Changes Proposed
Add
storage_data
reception, parsing and sending to/from the map-server.Remove loading and saving of
storage_data
through char.cRe-declaration of structure storage_data as a vector.
Re-code of portions in the map-server using
storage_data
.A new approach is taken by saving the loaded storage data from sql into memory for the duration of the session, thereby removing the need of querying the database to re-load all items everytime a storage save routine is issued from the map-server.
Saving of storage items is done through a new function that significantly reduces the number of queries compared to
char_memitemdata_tosql()
, and therefore run-time speed. This method could potentially reduce the number of update and delete queries fromMAX_STORAGE
(which could be >= 600) times to literally1
.Storage items are stored in a dynamically allocated array and handled accordingly.
struct mmo_charstatus
size reduces by34,800 bytes
.Affected Branches: Master
Issues addressed: #1669
Known Issues and TODO List