Implementation of Hercules Ultimate Storage System (HUSS) #3330

jasonch35 · 2024-11-04T11:18:51Z

Pull Request Prelude

I have followed proper Hercules code styling.
I have read and understood the contribution guidelines before making this PR.
I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Implementation of the Hercules Ultimate Storage System
Storages can now be created through a configuration file that describes their attributes.
Integrated storage_constants (Constant) into the configuration so that it can be assigned dynamically
Capacity is capped by MAX_STORAGE at config reading

Storage Configuration

{
        Id: (int)			Unique Identifier
        Name: (string)			Name of the storage sent to the client.
        Constant: (string)		Storage constant to be used by scripts
        Capacity: (int)			Maximum capacity of the storage.
}

Script Command
openstorage(<storage_constant>{, <storage_mode>});

Storage Modes

STORAGE_ACCESS_VIEW: View storage only.
STORAGE_ACCESS_GET: Allow retrieving items from storage.
STORAGE_ACCESS_PUT: Allow depositing items into storage.
STORAGE_ACCESS_ALL: (Default mode) Full access for viewing, retrieving, and depositing items.

All atcommands utilizing storage has been updated as well:

@storage <storage name/id>
@storeall <storage name/id>
@clearstorage <storage name/id>
@storagelist <storage name/id>

Issues addressed:

Upon testing the following has been fixed as well:

Bound items comparison #822
Bound items comparison #1659
Thank you @kyeme for pointing it out.

Credits to: @sagunkho and @dastgirp

Let's gooooo let's push this through!

This commit includes: - HUSS main implementation - openstorage script command update - @storage atcommand update - @clearstorage atcommand update - @storeall atcommand update - @storagelist atcommand update

- There can be an unlimited amount of storages and limits. - All setting names are case-sensitive and must be keyed accurately. - The storage capacity will default to MAX_STORAGE in mmo.h if it is set higher - Storage ID 1 is the default (official) storage for accounts. - Integrated storage_constants (Constant) into the configuration so that it can be assigned dynamically

vBrenth · 2024-11-06T15:56:45Z

Thank you @jasonch35 hope this one will get implemented.
Also i want to ask about Storage Limit its still 600 correct?

jasonch35 · 2024-11-07T18:46:34Z

Thank you @jasonch35 hope this one will get implemented.
Also i want to ask about Storage Limit its still 600 correct?

Yes, Limit is capped by MAX_STORAGE which is 600 by default.

jasonch35 · 2024-11-07T19:01:59Z

Btw, I kept the "default" storage without warning/errors as it is set as default in original Herc's branch.
Commands and scripts will result in "no storage found" message if it is removed/changed.
Or parameter error on server start if its constant isn't found.
I believe server owners should be responsible for adjusting the configuration file, just as they would if they manually remove Red_Potion in item_db.conf, they'd have to manually remove them in scripts if they choose to do so.

guilherme-gm

Thank you for putting the effort in fixing the previous PRs!
I did not try running it yet, but from the code review, I got those questions/points that may need to be addressed.

src/map/storage.c

sql-files/main.sql

src/common/msgtable.h

src/map/intif.h

src/char/int_storage.c

src/map/storage.c

src/common/mmo.h

src/map/storage.c

Reverted to local variables since its not needed to allocate anymore.

Adjusted callers

-Direct initialization for struct instead to NULL -Removed `intval` variable: copy value directly to `storage_id` -Removed unnecessary message table comments -Moved stst NULL check outside of if condition

+ few cleanups

Also added handler for when storage capacity is invalid or set below 1

-Added @storagelist error message and rearranged -Matched MSGTBL_STORAGE_NOT_LOADED to its messages.conf text

- Check duplicate for non imported storage ID and overwrite when imported ID exist - Move vector initialization on do_init_storage and transferred config reading after it

vBrenth · 2024-12-20T23:11:35Z

I know its Holidays but please give us this, we've been waiting for years. (officially)
@MishimaHaruna

guilherme-gm · 2024-12-20T23:37:56Z

src/char/int_storage.c

-
-		VECTOR_ENSURE(p->item, num_rows > MAX_STORAGE ? MAX_STORAGE : num_rows, 1);
+	if (SQL->NumRows(inter->sql_handle) > 0) {
+		VECTOR_ENSURE(p->item, MAX_STORAGE, 1);


this would be allocating more memory than needed if the storage is not full with MAX_STORAGE items. I think it would be better to keep the original logic of only allocating the needed space, never exceeding the MAX_STORAGE value.

as Haru stated above #3330 (comment), number of items showing in the storage should retrieve actual (or more) amount even if its higher than storage capacity and should be hard limited only by MAX_STORAGE. Instead, we let map server to handle the limits. Which I agree in case owners reduce the capacity of a specific storage.

I agree with this approach of hard limiting to MAX_STORAGE only, what I mean is that the current code would always allocate MAX_STORAGE items, even if the storage only has 1 item (not as a limit, but as current slots in use)

Syntax of VECTOR_ENSURE:
VECTOR_ENSURE(<Target Vector>, <How many empty spaces must be there>, <How much to increase if there is not enough spaces>)

If you check current master code, you have this:

VECTOR_ENSURE(p->item, num_rows > MAX_STORAGE ? MAX_STORAGE : num_rows, 1);

Ensure p->item has num_rows empty spaces, not exceeding MAX_STORAGE. Thus:

Let's say MAX_STORAGE = 600

if there are 5 items in storage, p->item will be a vector with 5 spaces (not 600)

if there are 600 items in storage, p->item will be a vector with 600 spaces (ok)

if there are 800 items in storage, p->item will be a vector with 600 spaces (hard limited)

In the new code:

VECTOR_ENSURE(p->item, MAX_STORAGE, 1);

Ensure p->item has MAX_STORAGE empty spaces. Thus:

Let's say MAX_STORAGE = 600

if there are 5 items in storage, p->item will be a vector with 600 spaces (595 unused ones)

if there are 600 items in storage, p->item will be a vector with 600 spaces (ok)

if there are 800 items in storage, p->item will be a vector with 600 spaces (hard limited)

I think we don't really need to allocate those 595 unused spaces on case 1. But I agree that we should get everything and hard limit at MAX_STORAGE.

guilherme-gm · 2024-12-21T00:11:40Z

src/map/storage.c

-	if (sd->storage.received == false) {
-		clif->message(sd->fd, msg_sd(sd, MSGTBL_STORAGE_NOT_LOADED)); // Storage has not been loaded yet.
-		return 1;
-	}


is removing the received check intended? from Haru message, he asked whether turning the message into assertion was intended (as an assertion doesn't inform the user why this happened).

But now, we would allow a not received storage to be "open".

Yes, I rewritten it as an assertion error but as Haru pointed out, it is no longer needed as the script and atcommands already errors out when called if storage is not loaded yet in intif_parse_account_storage

I think that if script/atcommand is already checking, it makes sense to switch from a message to an assert, but I would still keep the assert, because this is the real "entry point" to storage functions and we are expecting it to always have a received storage. This guards us from future issue if someone calls storageopen in a new place and forgets to add the received check.

For example, let's say official adds a new button to the UI that opens the storage wherever you are, I could end up calling storageopen from this new clif function without checking for received because I did not know/forgot.

It would be ok to give a "weird experience" like not working and not giving info to user, and generating an error in console (due to assert) that we would fix later on with proper checks. But it would be a big issue if the unreceived storage simply opened with bad data and later on overwrote the real storage, or the items added at this time disappeared.

If I understood, from Haru message, he was just checking whether the change from user message to "console error" (with assert) was intended, not that it was no longer needed, but I may be wrong.

@MishimaHaruna may you share your thoughts on this?

guilherme-gm · 2024-12-21T00:22:56Z

src/map/unit.c

+
+			for (int i = 0; i < VECTOR_LENGTH(sd->storage.list); i++) { // Storages
+				VECTOR_CLEAR(VECTOR_INDEX(sd->storage.list, i).item);
+				VECTOR_INDEX(sd->storage.list, i).received = false;


(adding here since the line is gone from the final diff)
the vector still needs to be cleared up, because its internal data is dynamically allocated. you just don't have to set received to false, as this data will be cleared up at VECTOR_CLEAR(sd->storage.list);

guilherme-gm · 2024-12-21T00:27:58Z

src/map/intif.c

 		ShowError("intif_parse_account_storage: Multiple calls from the inter-server received.\n");
 		return;
 	}

 	storage_count = (payload_size/sizeof(struct item));

-	VECTOR_ENSURE(sd->storage.item, storage_count, 1);
+	VECTOR_ENSURE(stor->item, storage_count, 1);


Memory manager reported a leak from this, I didn't check what is causing it (not sure if related to the missing VECTOR_CLEAR in unit.c

0001 : storage.c line 298 size 352 address 0x0x55976f3717e4 0002 : intif.c line 363 size 440 address 0x0x55977473f1fc

Added #3330 (comment), error now gone for me (I used builtin). Thank you!

guilherme-gm · 2024-12-21T00:28:58Z

src/map/storage.c

-		VECTOR_PUSH(sd->storage.item, *item_data);
-		it = &VECTOR_LAST(sd->storage.item);
+	if (i == VECTOR_LENGTH(stor->item)) {
+		VECTOR_ENSURE(stor->item, 1, 1);


Memory manager reported a leak from this, I didn't check what is causing it (not sure if related to the missing VECTOR_CLEAR in unit.c

0001 : storage.c line 298 size 352 address 0x0x55976f3717e4 0002 : intif.c line 363 size 440 address 0x0x55977473f1fc

Same as above

MishimaHaruna · 2024-12-31T17:14:34Z

src/map/script.c

@@ -12035,7 +12035,7 @@ static BUILDIN(openstorage)

 	sd->storage.access = storage_access; // Set storage access level. [Smokexyz/Hercules]


This is now redundant since it's done by storage->open()

Removed. thank you. please rereview.

src/map/chrif.c

vBrenth · 2025-01-07T14:48:53Z

Happy New Year @MishimaHaruna , @guilherme-gm Holidays are over, Please give us HERC user this feature. Thank you! its long-overdue.

jasonch35 added 8 commits November 4, 2024 18:50

Implementation of the Hercules Ultimate Storage System

1ffe6ad

This commit includes: - HUSS main implementation - openstorage script command update - @storage atcommand update - @clearstorage atcommand update - @storeall atcommand update - @storagelist atcommand update

Add SQL files for main db and upgrades

0a930b5

Add data on message tables for the updated atcommands

d403047

Update NPC scripts to comply with new command

95d6f76

Update script and atcommand documentation

98ad026

Update HPM Hooks

0cb567d

Show warning if storage capacity is over MAX_STORAGE

4aea3a4

guilherme-gm requested changes Nov 20, 2024

View reviewed changes

src/map/storage.c Show resolved Hide resolved

src/map/storage.c Outdated Show resolved Hide resolved

src/map/storage.c Show resolved Hide resolved

Use cart function when storing from cart

c8947dc

jasonch35 requested a review from guilherme-gm November 21, 2024 18:43

guilherme-gm approved these changes Nov 29, 2024

View reviewed changes

MishimaHaruna added this to the Release v2024.11 milestone Nov 30, 2024

MishimaHaruna requested changes Nov 30, 2024

View reviewed changes

MishimaHaruna modified the milestones: Release v2024.11, Release v2024.12 Nov 30, 2024

This was referenced Nov 30, 2024

Implementation of Hercules Ultimate Storage System (HUSS) #1763

Closed

Hercules Ultimate Storage System #2724

Closed

HUSS #3302

Closed

jasonch35 added 7 commits December 1, 2024 20:08

Indexed storage_id column from storage sql table

c5e746a

Char-server to ignore storage sizes and hardcoded to MAX_STORAGE instead

41528de

Reverted to local variables since its not needed to allocate anymore.

Moved storage_settings struct from mmo to storage header

6e485b3

Added access modes argument for storage->open

1dccefc

Adjusted callers

Refactored atcommands logic

8baf55b

-Direct initialization for struct instead to NULL -Removed `intval` variable: copy value directly to `storage_id` -Removed unnecessary message table comments -Moved stst NULL check outside of if condition

Missed name parameter in clif.c

973f6f6

Fixed invalid storage(0) assertion

dcdd63c

jasonch35 requested a review from MishimaHaruna December 1, 2024 14:57

jasonch35 force-pushed the huss branch from 4336a23 to beb162e Compare December 1, 2024 18:14

jasonch35 added 10 commits December 2, 2024 02:16

Indentation/space fix

49997aa

PCCHECK_STORAGE check fix

645b77a

Initialization logic fix

d6adf1b

+ few cleanups

Removed unnecessary cleaning of VECTOR

007a858

Removed full storage checks for functions that calls storage->additem()

1c34316

Set constant at the end of every iteration at config reading

a919fd8

Also added handler for when storage capacity is invalid or set below 1

Update HPM Hooks

53baf8b

Messagetable update

e882183

-Added @storagelist error message and rearranged -Matched MSGTBL_STORAGE_NOT_LOADED to its messages.conf text

Proper code styling for asterisks

dc85a3e

Clif update storage amount window to capacity

3ec152b

jasonch35 force-pushed the huss branch from beb162e to 3ec152b Compare December 1, 2024 18:18

jasonch35 added 2 commits December 2, 2024 02:24

Corrected documentation

8df6252

Storage config reading

9f08c77

- Check duplicate for non imported storage ID and overwrite when imported ID exist - Move vector initialization on do_init_storage and transferred config reading after it

guilherme-gm requested changes Dec 21, 2024

View reviewed changes

MishimaHaruna requested changes Dec 31, 2024

View reviewed changes

jasonch35 added 2 commits January 2, 2025 21:43

Removed redundant assignment

8eaf684

Clear item list for every storage at unit freeing

761ea97

jasonch35 force-pushed the huss branch from 9e25210 to 761ea97 Compare January 2, 2025 13:57

jasonch35 requested review from guilherme-gm and MishimaHaruna January 2, 2025 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of Hercules Ultimate Storage System (HUSS) #3330

Implementation of Hercules Ultimate Storage System (HUSS) #3330

jasonch35 commented Nov 4, 2024 •

edited

Loading

vBrenth commented Nov 6, 2024 •

edited

Loading

jasonch35 commented Nov 7, 2024

jasonch35 commented Nov 7, 2024 •

edited

Loading

guilherme-gm left a comment

vBrenth commented Dec 20, 2024

guilherme-gm Dec 20, 2024

jasonch35 Jan 2, 2025

guilherme-gm Jan 15, 2025

guilherme-gm Dec 21, 2024

jasonch35 Jan 2, 2025

guilherme-gm Jan 16, 2025

guilherme-gm Dec 21, 2024

guilherme-gm Dec 21, 2024

jasonch35 Jan 2, 2025 •

edited

Loading

guilherme-gm Dec 21, 2024

jasonch35 Jan 2, 2025

MishimaHaruna Dec 31, 2024

jasonch35 Jan 2, 2025

vBrenth commented Jan 7, 2025

		@@ -12035,7 +12035,7 @@ static BUILDIN(openstorage)

		sd->storage.access = storage_access; // Set storage access level. [Smokexyz/Hercules]

Implementation of Hercules Ultimate Storage System (HUSS) #3330

Are you sure you want to change the base?

Implementation of Hercules Ultimate Storage System (HUSS) #3330

Conversation

jasonch35 commented Nov 4, 2024 • edited Loading

Pull Request Prelude

Changes Proposed

vBrenth commented Nov 6, 2024 • edited Loading

jasonch35 commented Nov 7, 2024

jasonch35 commented Nov 7, 2024 • edited Loading

guilherme-gm left a comment

Choose a reason for hiding this comment

vBrenth commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonch35 Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vBrenth commented Jan 7, 2025

jasonch35 commented Nov 4, 2024 •

edited

Loading

vBrenth commented Nov 6, 2024 •

edited

Loading

jasonch35 commented Nov 7, 2024 •

edited

Loading

jasonch35 Jan 2, 2025 •

edited

Loading