Hercules Ultimate Storage System by dastgirp · Pull Request #2724 · HerculesWS/Hercules

dastgirp · 2020-05-16T18:02:41Z

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>})

Issues addressed:

Implementation of Hercules Ultimate Storage System (HUSS) #1763

4144 · 2020-05-16T19:01:05Z

from ci, look like shadow variable i

dastgirp · 2020-05-16T19:30:07Z

Windows compile fine for now, will be checking linux warnings and errors soon.

4144

some checks probably already added in last commit, but i checked commit by commit from start

Kenpachi2k13 · 2020-05-23T17:15:42Z

In my opinion the Xcode upgrade should not be part of this PR since it's not required.

dastgirp · 2020-05-24T05:30:59Z

In my opinion the Xcode upgrade should not be part of this PR since it's not required.

Ok, will remove it, I had just migrated it from old code to new code

dastgirp · 2020-05-24T06:25:42Z

@4144 @Kenpachi2k13 can you review again?

4144 · 2020-05-24T08:51:04Z

+			return;
+		}
+
+		nullpo_retv(stor);


this nullpo probably need move into if block, because after if block, stor is non null

4144 · 2020-05-24T08:52:58Z

+		struct storage_data *stor = storage->ensure(sd, sd->storage.current);
+		if (stor != NULL)
+			storage->get(sd, stor, item_index, item_amount);
+	} else if(sd->state.storage_flag == STORAGE_FLAG_GUILD) {


should be else if (sd->...

4144 · 2020-05-24T08:54:33Z

+	if ((stor = storage->ensure(sd, RFIFOW(fd, 8))) == NULL)
+		return;
+
+	Assert_retv(stor != NULL);


here stor is not null

4144 · 2020-05-24T08:55:10Z

+	if ((stor = storage->ensure(sd, storage_id)) == NULL)
+		return;
+
+	Assert_retv(stor != NULL);


and here stor is not null

bWolfie · 2020-06-08T08:49:35Z

nice bro

bWolfie · 2020-06-09T13:04:37Z

Anyone currently using this on their server?

4144 · 2020-07-26T02:09:27Z

@dastgirp you can complete this?

Vaans · 2020-10-28T19:05:28Z

dreams come true :D?

Kenpachi2k13 · 2020-11-05T22:09:41Z

 			if( sd->sc_display_count ) {
-				int i;
-				for(i = 0; i < sd->sc_display_count; i++) {
+				for(int i = 0; i < sd->sc_display_count; i++) {


Missing space after for and the curly brackets could be removed.

Kenpachi2k13 · 2020-11-05T22:10:34Z

 			}
 			VECTOR_CLEAR(sd->channels);
 			VECTOR_CLEAR(sd->script_queues);
+


Any reason for this empty line?

no, might be due to merge from old commit to here

Kenpachi2k13 · 2020-11-05T22:14:30Z


+/**
+ * Get a storage id by its name (through @commands etc...)
+ * @param[in] storage_name     pointer to the storage name char array.


Please separate the description and the parameter list with an empty comment line.

Kenpachi2k13 · 2020-11-05T22:15:35Z

+
+/**
+ * Get storage with a particular ID from a player.
+ * @param[in] sd         pointer to map session data.


Please separate the description and the parameter list with an empty comment line.

Kenpachi2k13 · 2020-11-05T22:17:52Z

+	int i = 0;
+	struct storage_data *stor = NULL;
+
+	nullpo_retr(NULL, sd);


Do the NULL pointer check prior to declaring/initialising the variables.

Kenpachi2k13 · 2020-11-06T00:09:45Z

 {
-	int payload_size = RFIFOW(fd, 2) - 8, account_id = RFIFOL(fd, 4);
+	int payload_size = RFIFOW(fd, 2) - 10, account_id = RFIFOL(fd, 4);
+	short storage_id = RFIFOW(fd, 8);


Suggested change

short storage_id = RFIFOW(fd, 8);

int storage_id = RFIFOW(fd, 8);

It is of 2 bytes, only, but as I see other are using int, changed it. done

Kenpachi2k13 · 2020-11-06T00:10:49Z

+ * @packet 0x3808     [out] <account_id>.L <storage_id>.W <save_flag>.B
 * @param  fd         [in]  File/Socket Descriptor.
 * @param  account_id [in]  Account ID of the storage in question.
 * @param  flag       [in]  Save flag, true for success and false for failure.


Add parameter storage_id.

Kenpachi2k13 · 2020-11-06T00:12:16Z

+	bool *matched_p = NULL;
+	int *delete = NULL;
 	int total_deletes = 0, total_updates = 0, total_inserts = 0;
-	int total_changes = 0;
+	int cp_size = 0;


Fix variable scopes.

Kenpachi2k13 · 2020-11-06T00:15:58Z

 CREATE TABLE IF NOT EXISTS `storage` (
  `id` INT UNSIGNED NOT NULL AUTO_INCREMENT,
  `account_id` INT UNSIGNED NOT NULL DEFAULT '0',
+  `storage_id` INT(11) UNSIGNED NOT NULL DEFAULT '1',


Suggested change

`storage_id` INT(11) UNSIGNED NOT NULL DEFAULT '1',

`storage_id` INT UNSIGNED NOT NULL DEFAULT '1',

Kenpachi2k13 · 2020-11-06T00:19:55Z

+-- You should have received a copy of the GNU General Public License
+-- along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+ALTER TABLE `storage` ADD `storage_id` INT(11)  UNSIGNED  NOT NULL  DEFAULT '1'  AFTER `account_id`;


Suggested change

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

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

SombraRO · 2020-11-19T14:36:30Z

@dastgirp something about that?

dastgirp · 2020-11-28T13:33:06Z

@Kenpachi2k13 updated to latest commit and made all changes that were mentioned by you.

4144 · 2020-11-28T14:26:41Z

look like compilation errors in all or most jobs

dastgirp · 2020-11-28T17:52:25Z

look like compilation errors in all or most jobs

was problem with scope, fixed it.

pazkero · 2021-02-15T21:07:33Z

@dastgirp Can you complete this? Seems to be having conflicts again... =(

dastgirp · 2021-02-16T04:56:32Z

@dastgirp Can you complete this? Seems to be having conflicts again... =(

It was already completed but for some reason not merged,
If it is going to be merged in next cycle, I can fix the conflicts again, but without confirmation, it would just keep on conflicting with every new release, so, I won't be fixing conflict as of now until confirmation is received or some changes are requested.

pazkero · 2021-02-16T07:39:03Z

By the way, quick question, is there a particular reason why you began storage IDs at 1 instead of 0 and did not made a default storage ID? (ie. When no storage ID is specified, default to 0 and open the basic storage)?Just curious, no need to rewrite anything on my account 😅As for milestones and etc I don't know, GitHub said it was waiting for @Kenpachi2k13 to review but I don't know if that's true or not =/But it might be related to why the PR has no tags and wasn't merged already?

pazkero · 2021-03-08T15:27:25Z

 {
 	struct map_session_data *sd = script->rid2sd(st);
-	if (sd == NULL)
+	int storage_id = script_getnum(st, 2);


Why not making this optional, as you need to have a storage 1 configured (SQL, for example) and considering it would improve compatibility?

Suggested change

int storage_id = script_getnum(st, 2);

int storage_id = script_hasdata(st, 2) ? script_getnum(st, 2) : 1;

Having this as optional is a way, but it won't tell others something is wrong.
Specifying storage id is better as it's clear that storage 1 will be used.
You can have any id you want, (even remove the Id 1), so having this optional would make it more confusing

sgsilva · 2021-07-01T15:10:18Z

Does anyone have any news about?

pazkero · 2021-07-01T15:41:59Z

It was already completed but for some reason not merged, If it is going to be merged in next cycle, I can fix the conflicts again, but without confirmation, it would just keep on conflicting with every new release, so, I won't be fixing conflict as of now until confirmation is received or some changes are requested.

It's officially on hold, I believe.

…

-- Sent from Yandex.Mail for mobile

12:10, July 1, 2021, Bruno Silva ***@***.***>: Does anyone have any news about? —You are receiving this because you commented.Reply to this email directly, view it on GitHub, or unsubscribe.

waken22 · 2021-11-05T04:31:29Z

Any news about this?

bWolfie · 2021-11-12T02:41:18Z

One day it will become a reality. Lol.

csnv · 2023-09-19T22:29:42Z

@dastgirp @MishimaHaruna can we have a definitive merging window and finish this?

Vaans · 2024-02-16T14:54:00Z

Four years already :'(

MishimaHaruna · 2024-11-30T22:07:09Z

Closing this in favor of the updated versions #3302, #3330

dastgirp requested review from 4144 and Kenpachi2k13 and removed request for Kenpachi2k13 May 16, 2020 18:02

dastgirp force-pushed the sStorage branch from c2fea08 to f25424e Compare May 16, 2020 19:08

dastgirp marked this pull request as draft May 16, 2020 19:29

4144 requested changes May 16, 2020

View reviewed changes

dastgirp force-pushed the sStorage branch 3 times, most recently from ab0a8dc to 772ff5e Compare May 24, 2020 06:09

dastgirp requested a review from 4144 May 24, 2020 06:11

dastgirp force-pushed the sStorage branch 2 times, most recently from 14871b7 to 274ec5b Compare May 24, 2020 06:24

dastgirp marked this pull request as ready for review May 24, 2020 06:24

dastgirp force-pushed the sStorage branch from 274ec5b to 90ccb6e Compare May 24, 2020 06:31

4144 requested changes May 24, 2020

View reviewed changes

Kenpachi2k13 requested changes Nov 6, 2020

View reviewed changes

smokexyz added 3 commits November 28, 2020 18:14

Add SQL Files for HUSS.

5fd2818

Update messages.conf with relevant messages to HUSS.

39fb0cd

Add storage.conf

2f532a2

smokexyz and others added 7 commits November 28, 2020 18:17

Added Storage modes to script constants

e99c68b

HPM Hook Update update

5f22404

Updated doc/script_commands.txt

9011dcd

Removed storagelist command from groups.conf TODO List :)

1526090

Updated scripts to comply with HUSS.

c9fa820

Followup with review edits.

b5aaef0

Changes to mapif and merge fixes

aa8a87e

dastgirp force-pushed the sStorage branch from 90ccb6e to ebdb684 Compare November 28, 2020 13:32

dastgirp requested review from 4144 and Kenpachi2k13 November 28, 2020 13:32

4144 approved these changes Nov 28, 2020

View reviewed changes

Code styling

26f4ec8

dastgirp force-pushed the sStorage branch from ebdb684 to 26f4ec8 Compare November 28, 2020 17:45

pazkero reviewed Mar 8, 2021

View reviewed changes

Samuel23 mentioned this pull request Jun 18, 2024

HUSS #3302

Closed

3 tasks

jasonch35 mentioned this pull request Nov 4, 2024

Implementation of Hercules Ultimate Storage System (HUSS) #3330

Open

8 tasks

MishimaHaruna closed this Nov 30, 2024

	short storage_id = RFIFOW(fd, 8);
	int storage_id = RFIFOW(fd, 8);

	`storage_id` INT(11) UNSIGNED NOT NULL DEFAULT '1',
	`storage_id` INT UNSIGNED NOT NULL DEFAULT '1',

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

	int storage_id = script_getnum(st, 2);
	int storage_id = script_hasdata(st, 2) ? script_getnum(st, 2) : 1;

Conversation

dastgirp commented May 16, 2020 • edited by jguy1987 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Prelude

Changes Proposed

Design

Access Modes

Script Commands

Uh oh!

4144 commented May 16, 2020

Uh oh!

dastgirp commented May 16, 2020

Uh oh!

4144 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kenpachi2k13 commented May 23, 2020

Uh oh!

dastgirp commented May 24, 2020

Uh oh!

dastgirp commented May 24, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bWolfie commented Jun 8, 2020

Uh oh!

bWolfie commented Jun 9, 2020

Uh oh!

4144 commented Jul 26, 2020

Uh oh!

Vaans commented Oct 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dastgirp commented May 16, 2020 •

edited by jguy1987

Loading