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.

Sign up for GitHub

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

Jump to bottom

fix: convert golden and royal costume outfit from storage to kv #2477

Closed
wants to merge 23 commits into from
Closed

fix: convert golden and royal costume outfit from storage to kv #2477

wants to merge 23 commits into from

Conversation

omarcopires
Copy link
Contributor

@omarcopires omarcopires commented Mar 22, 2024 •
edited
Loading

This pull request introduces a series of significant improvements and optimizations to the canary datapack. The changes include:

  • Addition of new promotion NPC: A new promotion NPC has been added to the canary datapack to enhance the player experience.
  • Removal of unused storages: Unused storages have been identified and removed from the library, optimizing resource usage and simplifying the codebase.
  • Cleanup of Portuguese comments: Portuguese comments have been removed to maintain consistency and facilitate code comprehension for a wider audience.
  • Improvement of money removal logic: The logic for money removal has been enhanced to ensure more efficient and accurate operation.
  • Migration to KV for storage: Storages have been migrated to KV for improved efficiency and data management, providing a more scalable and robust solution.
  • Correction of incorrect outfit display attributes: Adjustments have been made to incorrect outfit display attributes to ensure accurate and consistent rendering.
  • Enhancement of code readability: Code readability has been improved by removing hardcode and utilizing constants, making it more understandable and easier to maintain.
  • Addition of migration for player storage: A migration has been added to store information for players who had the previous storage, ensuring a smooth transition without data loss.

@omarcopires omarcopires marked this pull request as ready for review March 22, 2024 03:30
@elsongabriel elsongabriel changed the title chore: convert golden outfit from storage to kv fix: convert golden outfit from storage to kv Mar 22, 2024
@elsongabriel
Copy link
Contributor

elsongabriel commented Mar 22, 2024 •
edited
Loading

When you change the storage type usage, remember to create migrations to update the players with this outfit.

And please create a description of this PR.

@omarcopires
Copy link
Contributor Author

When you change the storage type usage, remember to create migrations to update the players with this outfit.

And please create a description of this PR.

There is no description because it is not ready, there are still many things missing.

@omarcopires omarcopires marked this pull request as draft March 22, 2024 15:23
@omarcopires omarcopires force-pushed the golden-outfit-to-kv branch 2 times, most recently from 2ef1aec to 93d85d0 Compare March 22, 2024 16:02
@omarcopires omarcopires force-pushed the golden-outfit-to-kv branch from 93d85d0 to e8b5bef Compare March 22, 2024 16:02
@omarcopires omarcopires force-pushed the golden-outfit-to-kv branch 2 times, most recently from 3fe8332 to 0ec432d Compare March 22, 2024 16:36
@omarcopires omarcopires force-pushed the golden-outfit-to-kv branch from 01eeaf8 to ba81b3e Compare March 22, 2024 16:38
@omarcopires omarcopires changed the title fix: convert golden outfit from storage to kv fix: convert golden and royal costume outfit from storage to kv Mar 22, 2024
@omarcopires omarcopires force-pushed the golden-outfit-to-kv branch 3 times, most recently from d634da8 to 5ae9685 Compare March 22, 2024 17:51
@omarcopires omarcopires force-pushed the golden-outfit-to-kv branch 2 times, most recently from 8331ecd to 932dcfd Compare March 22, 2024 18:07
@omarcopires omarcopires marked this pull request as ready for review March 22, 2024 18:08
@omarcopires
Copy link
Contributor Author

Unfortunately, I couldn't migrate the outfit display to receive the KVs. Perhaps someone here or in the future can do it.

@luanluciano93
Copy link
Contributor

Does not need a cache to be checked every 60 seconds, and we know when it is updated (when the addon is added), so wait for #2404 to be merged.

elsongabriel
elsongabriel reviewed Apr 18, 2024
View reviewed changes
Copy link
Contributor

@elsongabriel elsongabriel left a comment

Choose a reason for hiding this comment

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

If you want you can unify the functions that will check these outfits in one file, and these npcs will call this function "check" and do the action.

data-otservbr-global/npc/emperor_kruzak.lua Outdated Show resolved Hide resolved
data-otservbr-global/npc/emperor_kruzak.lua Outdated Show resolved Hide resolved
data-otservbr-global/npc/emperor_kruzak.lua Outdated Show resolved Hide resolved
data-otservbr-global/npc/king_tibianus.lua Outdated Show resolved Hide resolved
data-otservbr-global/npc/king_tibianus.lua Outdated Show resolved Hide resolved
data-otservbr-global/npc/percybald.lua Outdated Show resolved Hide resolved
data-otservbr-global/npc/percybald.lua Outdated Show resolved Hide resolved
data-otservbr-global/npc/queen_eloise.lua Outdated Show resolved Hide resolved
data-otservbr-global/npc/queen_eloise.lua Outdated Show resolved Hide resolved
data-otservbr-global/npc/queen_eloise.lua Outdated Show resolved Hide resolved
@omarcopires
Copy link
Contributor Author

If you want you can unify the functions that will check these outfits in one file, and these npcs will call this function "check" and do the action.

I wouldn't find it necessary to create a function since some NPCs have slightly different dialogue lines, which might cause complications. Moreover, maintaining the difficulty level would be challenging if I were to add the function and duplicate the same code within those NPCs that have extra dialogues.

@omarcopires omarcopires force-pushed the golden-outfit-to-kv branch 2 times, most recently from 3b781c8 to 4cd971c Compare April 22, 2024 17:07
@omarcopires omarcopires force-pushed the golden-outfit-to-kv branch from 4cd971c to ec68087 Compare April 25, 2024 16:25
@majestyotbr
Copy link
Contributor

Is this ready for tests?

elsongabriel
elsongabriel reviewed Jun 12, 2024
View reviewed changes
data-otservbr-global/lib/core/storages.lua
@@ -879,8 +879,7 @@ Storage = {
AssassinBaseOutfit = 51012,
AssassinFirstAddon = 51013,
AssassinSecondAddon = 51014,
-- Golden Outfit
GoldenOutfit = 51015,
-- Nightmare Outfit
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- Nightmare Outfit
-- GoldenOutfit = 51015, (moved to kv)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no need, as it is free storage, it was set to -1 in the migration

elsongabriel
elsongabriel reviewed Jun 12, 2024
View reviewed changes
data-otservbr-global/lib/core/storages.lua
@@ -893,7 +892,6 @@ Storage = {
DeeplingAnchor = 51023,
FirstOrientalAddon = 51024,
SecondOrientalAddon = 51025,
RoyalCostumeOutfit = 51026,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RoyalCostumeOutfit = 51026,
-- RoyalCostumeOutfit = 51026, (moved to kv)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no need, as it is free storage, it was set to -1 in the migration

@omarcopires
Copy link
Contributor Author

Is this ready for tests?

YES!

@luanluciano93
Copy link
Contributor

Are you removing the outfit memorial?

@majestyotbr
Copy link
Contributor

majestyotbr commented Jul 10, 2024 •
edited
Loading

Update your pull request and use the pull request template.

@majestyotbr
Copy link
Contributor

Why did you deleted the outfit memorial action?

@majestyotbr
Copy link
Contributor

For your PR to be approved, you need to add back the outfit memorial action with kv and correct typo in the goldenOutfitStorageValue.

@omarcopires omarcopires deleted the golden-outfit-to-kv branch July 17, 2024 17:52
@omarcopires
Copy link
Contributor Author

I don't have the time or motivation to continue with this pull request. A lot of time has passed, and I have other priorities now. Feel free to continue if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elsongabriel elsongabriel elsongabriel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@omarcopires @elsongabriel @luanluciano93 @majestyotbr