HUSS #3302

Samuel23 · 2024-06-18T05:47:52Z

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

Design

Implementation of the Hercules Ultimate Storage System
Storages can now be created through a configuration file that describes their attributes.
Example storage configuration:

{
        Id: (int)                              (required|unique) Unique Identifier
        Name: (string)                         (required) Name of the storage sent to the client.
        Capacity: (int)                        (required) Maximum capacity of the storage.
}

Additional storages are handled with dynamic arrays that will save a tonne of memory when created, as opposed to a design in which they were implemented using fixed length arrays. In simple terms, a storage of 600 items would approximately cost the same amount of memory as 600 storages with 1 item each.
They are saved in the same storage database (SQL) as the original separating them by a storage identifier.
An infinite number of storages can be created, there are no limits.
The current design implementation only allow saving/loading of approximately 1600 items per storage due to packet size limits.

PS. Make sure you apply SQL upgrades for this patch!

Access Modes

Storage access modes can be set through the openstorage builtin command.

	STORAGE_ACCESS_VIEW     // View storage only
	STORAGE_ACCESS_GET      // Allow getting items from storage.
	STORAGE_ACCESS_PUT      // Allow putting items to storage.
	STORAGE_ACCESS_ALL      // Allow all actions.

Default storage mode : STORAGE_ACCESS_ALL

Script Commands

Changed: openstorage(<storage_id>{, <storage_mode>})

Credits still to: @sagunkho and @dastgirp
Just updated and tested with latest repository version
Issues addressed:

[x] #1763
[x] #2724

Added missing sql upgrade

guilherme-gm

Hello, thank you for your PR!

I did an initial review that hopefully covers most of the changes. But I did not test it.

Now to some things not mentioned in the review messages (or only briefly mentioned)

Concern regarding storage sizes

I have some concern regarding misconfiguration of storages, since as far as I can tell:

The inter-server packet does not support splitting, so it is limited to a 65k bytes packet. If a single storage has more items than it fits this space, it would cause issues
The client packet does not support splitting too, so the same applies.

I remember in current code we have an assertion to prevent it from overflowing, but with this new code I don't think we have this check (at least I could not find it). I believe we should have some sort of safe guarding to prevent item losses/misconfiguration.

I guess the easiest solution here would be to calculate how many items are supported in the running server (probably the smallest number between inter-server and client) and limit storage config to now go over this value. If in the future one of those grow and the current user config no longer supports it, they would be informed and could plan the upgrade for their server.

Regarding commits

It would be nice if this was split into commits so that if in the future we'll need to debug an issue near the lines of code edited here, we'll be able to figure out if a change was intentional or accidental by reading the commit messages.

I can help splitting it if you can't or don't know how.

My suggestion regarding how to split it is:

Convert a single storage from array to vector
Add storage config loading (but not being used yet)
Add multiple storages (at this moment, let openstorage command just open the storage id 1)
Update openstorage with storage id param + NPCs (No access code for now)
Add access type parameter
Update other atcommands (maybe with 1 commit for each changed command)

(If some changes doesn't fall into any of those, make a separate commit)

Again, thank you for your contribution and feel free to ask if needed :)

guilherme-gm · 2024-06-28T01:15:53Z

conf/groups.conf

@@ -235,7 +235,6 @@ groups: (
 		jail: true
 		jailfor: true
 		mute: true
-		storagelist: true


is the removal of storagelist permission and @storagelist atcommand intentional? I did not see this mention on the PR nor commit message about why this atcommand is being removed

I mean, this PR is just #2724 rebased, so it has the same caveats as the original. Which includes no default storage and not having implemented some conveniences like @storagelist command. Do keep in mind this got the point of "no changes requested by team" before being entirely forgotten by said team and rendered un-mergeable. Most issues you pointed out either weren't considered an issue in the original or are just maintenance upkeep. Large, complex, non-urgent PRs like this one suffer from an unreasonably large maintenance upkeep. Usually something with only two or one or none of these three attributes get in, breaking the PR and thus the high maintenance upkeep. Honorable mention to #466 which is another good example of death by upkeep.

...Although for these design decisions you can ask @dastgirp as they are the original author, I guess? I don't think they'll remember much though, it has been four years already.

(To be more clear and if memory serves as it's been a long time since I looked at the original, @storagelist will not work with this PR short of a total rewrite of the command, so it was decided to do a regression and remove the command, as it's seldom used and out of scope, from the discussions if it should list all storages or take an argument, up to the actual implementation which would add even more stuff to review in an already large PR. There was no objection from the original reviewers).

guilherme-gm · 2024-06-28T01:16:27Z

conf/map/storage.conf

+	Name: "Storage"
+	Capacity: 600
+},
+)


Please add an empty line at the end of the file, this makes it friendlier for diffs/version control systems.

guilherme-gm · 2024-06-28T01:16:36Z

conf/map/storage.conf

+}
+*******************************************************************************/
+{
+    // DO NOT EDIT THIS ENTRY!


indent issue here

guilherme-gm · 2024-06-28T01:18:09Z

sql-files/upgrades/2024-06-18--12-32.sql

+
+ALTER TABLE `storage` ADD `storage_id` INT UNSIGNED NOT NULL DEFAULT '1' AFTER `account_id`;
+
+INSERT INTO `sql_updates` (`timestamp`) VALUES (1718685120);


please add an empty line at the end of the file

guilherme-gm · 2024-06-28T01:20:46Z

src/common/mmo.h

 struct storage_data {
+	int uid;					   ///< Storage Identifier.


indent issue

guilherme-gm · 2024-06-28T23:57:33Z

src/map/atcommand.c

+		}
+	}
+	else {
+		clif->message(fd, msg_fd(fd, 68)); // Please specify a storage name (usage: @storage <storage name>).


please use MSGTBL_ constants here

guilherme-gm · 2024-06-29T00:00:49Z

src/map/atcommand.c

+	if (*message != '\0' && sscanf(message, "%12d", &intval) == 1) {
+		if (storage->get_settings(intval) == NULL) {
+			snprintf(atcmd_output, sizeof(atcmd_output), msg_fd(fd, 67), storage_name);
+			clif->message(fd, atcmd_output); // Invalid storage name or ID.
+			return false;
+		}
+		storage_id = intval;
+	}
+	else if (*message != '\0' && sscanf(message, "%23s", storage_name) == 1) {
+		if ((storage_id = storage->get_id_by_name(storage_name)) == -1) {
+			snprintf(atcmd_output, sizeof(atcmd_output), msg_fd(fd, 67), storage_name);
+			clif->message(fd, atcmd_output); // Invalid storage name or ID.
+			return false;
+		}
+	}
+	else {
+		clif->message(fd, msg_fd(fd, 65)); // Please specify a storage name (usage: @storeall <storage name>).
+		return false;
+	}


same here for previous review messages:

fix style in else if/else

use MSGTBL_* constants

guilherme-gm · 2024-06-29T00:02:05Z

src/map/atcommand.c

+	if (*message != '\0' && sscanf(message, "%12d", &intval) == 1) {
+		if (storage->get_settings(intval) == NULL) {
+			snprintf(atcmd_output, sizeof(atcmd_output), msg_fd(fd, 67), storage_name);
+			clif->message(fd, atcmd_output); // Invalid storage name or ID.
+			return false;
+		}
+		storage_id = intval;
+	}
+	else if (*message != '\0' && sscanf(message, "%23s", storage_name) == 1) {
+		if ((storage_id = storage->get_id_by_name(storage_name)) == -1) {
+			snprintf(atcmd_output, sizeof(atcmd_output), msg_fd(fd, 67), storage_name);
+			clif->message(fd, atcmd_output); // Invalid storage name or ID.
+			return false;
+		}
+	}
+	else {
+		clif->message(fd, msg_fd(fd, 66)); // Please specify a storage name (usage: @clearstorage <skill name>).
+		return false;
+	}


same here for previous review messages:

fix style in else if/else

use MSGTBL_* constants

guilherme-gm · 2024-06-29T00:03:24Z

src/map/atcommand.c

-	if( strcmpi(info->command, "storagelist") == 0 ) {
-		location = "storage";
-		items = VECTOR_DATA(sd->storage.item);
-		size = VECTOR_LENGTH(sd->storage.item);
-	} else if( strcmpi(info->command, "cartlist") == 0 ) {


why is the command being removed?

guilherme-gm · 2024-06-29T00:12:20Z

src/map/intif.c

-		ShowError("intif_parse_account_storage_save_ack: Storage has not been saved! (AID: %d)\n", account_id);
-		sd->storage.save = true; // Flag it as unsaved, to re-attempt later


Is it intended that a failure is no longer retried? Per my understanding, a failure would be eventually retried in the original code, but now if it fails, it will never be saved (potentially leading to a exploit/item loss)

vBrenth · 2024-09-04T21:02:58Z

Still waiting for this to get implemented officially.

SombraRO · 2024-11-01T00:35:56Z

In 5 years maybe this will be implemented

vBrenth · 2024-11-01T06:06:33Z

In 5 years maybe this will be implemented

Hahahahahahaha, Well i have this on my emulator already. Its just i want to make this one officially implemented. Im using it for like 2 years now.

Samuel23 added 3 commits June 18, 2024 12:45

Updated Hercules Ultimate Storage System to latest version

a111264

Revert encoding to original

5af72df

Encoding Fixed

e3f2e13

Added missing sql upgrade

guilherme-gm self-requested a review June 28, 2024 02:55

guilherme-gm requested changes Jun 29, 2024

View reviewed changes

guilherme-gm changed the base branch from stable to master June 29, 2024 22:48

MishimaHaruna added this to the Release v2024.07 milestone Jun 30, 2024

MishimaHaruna modified the milestones: Release v2024.07, Release v2024.08 Aug 1, 2024

MishimaHaruna modified the milestones: Release v2024.09, Release v2024.10 Sep 30, 2024

MishimaHaruna modified the milestones: Release v2024.10, Release v2024.11 Oct 31, 2024

jasonch35 mentioned this pull request Nov 4, 2024

Implementation of Hercules Ultimate Storage System (HUSS) #3330

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HUSS #3302

HUSS #3302

Samuel23 commented Jun 18, 2024

guilherme-gm left a comment •

edited

Loading

guilherme-gm Jun 28, 2024

pazkero Aug 10, 2024

pazkero Aug 10, 2024

guilherme-gm Jun 28, 2024

guilherme-gm Jun 28, 2024

guilherme-gm Jun 28, 2024

guilherme-gm Jun 28, 2024

guilherme-gm Jun 28, 2024

guilherme-gm Jun 29, 2024

guilherme-gm Jun 29, 2024

guilherme-gm Jun 29, 2024

guilherme-gm Jun 29, 2024

vBrenth commented Sep 4, 2024

SombraRO commented Nov 1, 2024

vBrenth commented Nov 1, 2024


		ALTER TABLE `storage` ADD `storage_id` INT UNSIGNED NOT NULL DEFAULT '1' AFTER `account_id`;

		INSERT INTO `sql_updates` (`timestamp`) VALUES (1718685120);

		ShowError("intif_parse_account_storage_save_ack: Storage has not been saved! (AID: %d)\n", account_id);
		sd->storage.save = true; // Flag it as unsaved, to re-attempt later

HUSS #3302

Are you sure you want to change the base?

HUSS #3302

Conversation

Samuel23 commented Jun 18, 2024

Pull Request Prelude

Changes Proposed

Design

Access Modes

Script Commands

guilherme-gm left a comment • edited Loading

Choose a reason for hiding this comment

Concern regarding storage sizes

Regarding commits

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

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 Sep 4, 2024

SombraRO commented Nov 1, 2024

vBrenth commented Nov 1, 2024

guilherme-gm left a comment •

edited

Loading