Skip to content
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

Implementation of Hercules Ultimate Storage System (HUSS) #1763

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

sagunkho
Copy link
Member

@sagunkho sagunkho commented Jun 4, 2017

Pull Request Prelude

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

Affected Branches: master

Issues addressed: #1762

Known Issues and TODO List

  • Allow resending packets with remaining item data that exceeds packet size (in the case where storage capacity is above the limit).
  • Alternatively, Screw sending any storage details to the character server and handle everything on the map server level.
  • Test storage modes.
  • Storage names are only sent client side when an item is present in the storage. (part of the clif_storagelist clause in storage_storageopen

@MishimaHaruna MishimaHaruna added the status:inprogress Issue is being worked on / the pull request is still a WIP label Jun 4, 2017
@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@sagunkho sagunkho force-pushed the storages branch 2 times, most recently from 9629ee6 to 429b8c4 Compare June 4, 2017 16:57
@sagunkho sagunkho added the status:code-review Awaiting code review label Jun 4, 2017
@sagunkho sagunkho force-pushed the storages branch 3 times, most recently from 9a60418 to 7a4989c Compare June 6, 2017 13:48
@sagunkho sagunkho removed the status:inprogress Issue is being worked on / the pull request is still a WIP label Jun 17, 2017
@Helianthella Helianthella moved this from Stalled / Forgotten to Release Candidate (review) in Project Board Aug 5, 2017
@Helianthella Helianthella added the status:needs-rebase The PR has diverged from the stable branch and needs to be rebased label Oct 8, 2017
@Helianthella Helianthella added this to the future release milestone Oct 8, 2017
@MishimaHaruna MishimaHaruna modified the milestones: future release, Release v2017.11.19 Oct 22, 2017
@MishimaHaruna MishimaHaruna modified the milestones: Release v2017.11.19, future release, Release v2017.12.17 Nov 18, 2017
@Jedzkie
Copy link
Contributor

Jedzkie commented Nov 28, 2017

@Smokexyz any news on this?

@@ -64,7 +64,7 @@
// 20120307 - 2012-03-07aRagexeRE+ - 0x970

#ifndef PACKETVER
#define PACKETVER 20141022
#define PACKETVER 20150513
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you change it for testing or the default PACKETVER will really change? If it will change, I think it should be in a separate commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep this was for testing only, I forgot to revert the change.

@@ -5537,9 +5539,15 @@ storage window, to avoid any disruption when both windows overlap.

mes("I will now open your stash for you");
close2();
openstorage();
openstorage(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to use openstorage(STORAGE_TYPE_MAIN) with the new constant for storage type.

@MishimaHaruna MishimaHaruna modified the milestones: Release v2017.12.17, future release Dec 17, 2017
@sagunkho
Copy link
Member Author

sagunkho commented Dec 23, 2017

@HerculesWS/developers Is there anything to be done on this? If so, I can work on it as I rebase for conflict resolution. @4144 @MishimaHaruna

int i = 0;
struct storage_data *stor = NULL;

nullpo_retr(NULL, sd);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here need return not NULL but some kind of default storage.
Because in other code no checks for return from storage->ensure

{
struct item_data *data = NULL;
struct item *it = NULL;
const struct storage_settings *stst = storage->get_settings(stor->uid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing nullpo check for stor variable

{
const struct storage_settings *stst = storage->get_settings(stor->uid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing nullpo check for stor variable

{
const struct storage_settings *stst = storage->get_settings(stor->uid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing nullpo check for stor variable.
Check after this line is useless, because here server already crashed

{
const struct storage_settings *stst = storage->get_settings(stor->uid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here missing nullpo check for stor too.
Next check for stor is useless

{
int flag = 0;
struct item *it = NULL;

nullpo_ret(sd);

Assert_ret(sd->storage.received == true);
Assert_ret(stor->received == true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing nullpo for stor variable

@MishimaHaruna MishimaHaruna modified the milestones: future release, Release v2018.01.14 Dec 23, 2017
@ghost ghost assigned sagunkho Dec 26, 2017
@sagunkho sagunkho removed the status:needs-rebase The PR has diverged from the stable branch and needs to be rebased label Jan 6, 2018
@MishimaHaruna MishimaHaruna modified the milestones: Release v2018.01.14, future release, Release v2018.02.11 Jan 14, 2018
@xBeret
Copy link

xBeret commented Jan 21, 2018

@Smokexyz If you have some time please look at PR, thanks.

@SombraRO
Copy link

@Asheraf you think of rebase in this in that PR, it seems that it exists in the official a long time?

@marcosbucarte
Copy link

@dastgirp Do you think of rebase? using more than one storage is good for private server.

@dastgirp
Copy link
Member

I forgot that this wasn't merged, will see what I can do

@marcosbucarte
Copy link

Thanks @dastgirp for looking at this.

@sagunkho sagunkho reopened this Jan 16, 2019
@SombraRO
Copy link

SombraRO commented Mar 3, 2019

@dastgirp @4144 @Asheraf @MishimaHaruna @sagunkho Something about it? it would be interesting to implement this in hercules

@dastgirp dastgirp self-assigned this Mar 9, 2019
@AnnieRuru
Copy link
Contributor

just wanna tell this is already in rathena -> rathena/rathena@46b1de7
just without the memory slasher, means content-wise we are lagging behind
https://rathena.org/board/topic/115116-personal-char-storage/

@tlacson7
Copy link

tlacson7 commented Apr 1, 2019

@dastgirp yeah boi!!! your the man!

@Zarbony
Copy link
Contributor

Zarbony commented May 30, 2019

Umm will this System merged?
I need the option to add more storages :)
Still rAthena got this already and i change my running system now to Hercules

@4144
Copy link
Contributor

4144 commented May 30, 2019

Umm will this System merged?

not yet merged, here code too outdated...

@dastgirp dastgirp removed their assignment May 16, 2020
@Samuel23 Samuel23 mentioned this pull request Jun 18, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code-review Awaiting code review
Projects
Project Board
Release Candidate (review)
Development

Successfully merging this pull request may close these issues.

None yet