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

MishimaHaruna · 2024-11-30T18:23:00Z

src/char/int_storage.c

 	if (VECTOR_LENGTH(cp.item) > 0) {
+
+		int *delete = aCalloc(cp_size, sizeof(int));


This could be a VECTOR as well, removing the need to allocate it to an arbitrarily large size. It can be expanded as items to be deleted are found, with an appropriate growth step.
This would remove the need for a couple of confusing 'size' support vars

Reverted to local variables since we hardcoded to a fixed MAX_STORAGE anyway.
I think it'll be much more efficient this way.

MishimaHaruna · 2024-11-30T18:23:36Z

src/char/int_storage.c

+
+	int cp_size = inter_storage->fromsql(account_id, storage_id, &cp, 0);
+
+	bool* matched_p = aCalloc(VECTOR_LENGTH(p->item), sizeof(bool));


This is a good candidate for a VECTOR, to avoid doing array subscript on a bare pointer and to make its length clear to the reader

Same as above:

Reverted to local variables since we hardcoded to a fixed MAX_STORAGE anyway.
I think it'll be much more efficient this way.

MishimaHaruna · 2024-11-30T20:37:11Z

src/map/storage.c

+	nullpo_retr(false, filename);
+
+	if (!imported)
+		VECTOR_INIT(storage->configuration);


Can you change this to VECTOR_CLEAR() and move the init to storage_defaults() so that there are no uninitialized variables

~~I've been thinking. I wanna drop this feature altogether. Since storage.conf will be as is almost forever officially similar to other configs. There is no point having additional fields and imports.~~ Nvm.

9f08c77

I transferred config read function below the initialization.

MishimaHaruna · 2024-11-30T20:43:45Z

src/common/mmo.h

+/* Hercules Ultimate Storage System [Smokexyz/Hercules] */
+struct storage_settings {
+	int uid;                       ///< Storage Identifier.
+	char name[NAME_LENGTH];        ///< Storage Name
+	int capacity;                  ///< Item Capacity.
+};


This belongs to the map server and the char server has no business with it. Please move it to storage.h

6e485b3

Also regenerated HPM for this.

src/map/storage.c

MishimaHaruna · 2024-11-30T21:59:22Z

src/map/storage.c

+		storage->config_read_additional_fields(t, &s_conf, filename);
+
+		if (imported) {
+			ARR_FIND(0, VECTOR_LENGTH(storage->configuration), i, VECTOR_INDEX(storage->configuration, i).uid == s_conf.uid);


As it stands, this seems an impossible condition (the duplicate check above was designed to only be executed if imported == false perhaps?)

Same as above:
I've been thinking. I wanna drop this feature altogether. Since storage.conf will be as is almost forever officially similar to other configs. There is no point having additional fields and imports. Nvm.

9f08c77

Added check on the first duplicate check.

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.

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".

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

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

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

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

MishimaHaruna Nov 30, 2024

jasonch35 Dec 1, 2024 •

edited

Loading

MishimaHaruna Nov 30, 2024

jasonch35 Dec 1, 2024 •

edited

Loading

MishimaHaruna Nov 30, 2024

jasonch35 Dec 1, 2024 •

edited

Loading

jasonch35 Dec 11, 2024

MishimaHaruna Nov 30, 2024

jasonch35 Dec 1, 2024

MishimaHaruna Nov 30, 2024

jasonch35 Dec 1, 2024 •

edited

Loading

jasonch35 Dec 11, 2024

vBrenth commented Dec 20, 2024

guilherme-gm Dec 20, 2024

guilherme-gm Dec 21, 2024

guilherme-gm Dec 21, 2024

guilherme-gm Dec 21, 2024

guilherme-gm Dec 21, 2024

		if (VECTOR_LENGTH(cp.item) > 0) {

		int *delete = aCalloc(cp_size, sizeof(int));


		int cp_size = inter_storage->fromsql(account_id, storage_id, &cp, 0);

		bool* matched_p = aCalloc(VECTOR_LENGTH(p->item), sizeof(bool));

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

Choose a reason for hiding this comment

jasonch35 Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonch35 Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonch35 Dec 1, 2024 • 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

jasonch35 Dec 1, 2024 • edited Loading

Choose a reason for hiding this 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

jasonch35 commented Nov 4, 2024 •

edited

Loading

vBrenth commented Nov 6, 2024 •

edited

Loading

jasonch35 commented Nov 7, 2024 •

edited

Loading

jasonch35 Dec 1, 2024 •

edited

Loading

jasonch35 Dec 1, 2024 •

edited

Loading

jasonch35 Dec 1, 2024 •

edited

Loading

jasonch35 Dec 1, 2024 •

edited

Loading