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

Feature/gp shop #644

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

pacampbell
Copy link
Collaborator

@pacampbell pacampbell commented Nov 29, 2024

Features:

  • Allow purchasing GG for CAP (currently for free).
  • Allow drawing from "normal treasure lot" gacha to receive items.
  • Allow drawing from "box treasure lot" gacha to receive items.
  • Enable event code input.

Technical details:

  • Implements BOX_GACHA_BOX_GACHA_BUY_REQ/RES
  • Implements BOX_GACHA_BOX_GACHA_DRAW_INFO_REQ/RES
  • Implements BOX_GACHA_BOX_GACHA_LIST_REQ/RES
  • Implements GACHA_GACHA_BUY_REQ/RES
  • Implements GACHA_GACHA_LIST_REQ/RES
  • Implements EVENT_CODE_EVENT_CODE_INPUT_REQ/RES
  • Implements GP_CHANGE_CAP_TO_GP_REQ/RES
  • Implements GP_COG_GET_ID_REQ/RES
  • Implements GP_GP_SHOP_DISPLAY_GET_TYPE_REQ/RES
  • Implements ITEM_GET_ITEM_STORAGE_INFO_REQ/RES

Notes on testing:

  • From the menu open the relevant shop-related menu points and buy gacha items, type in event code etc.
  • Online Shop
  • Check Golden Gemstones
  • Treasure Lot
  • Event Code Input

Open points:

  • Introduction of assets for box gacha, normal gacha, event codes and replacing hard-coded values
  • Getting CAP balance to work in-game
  • Online Shop closes client, maybe malformed packets
  • Finding more old images for campaigns/banners

Authored-by: @Sehkah

Checklist:

  • The project compiles
  • The PR targets develop branch

@alborrajo
Copy link
Collaborator

Any reason this hasn't been merged? 🤔

@pacampbell
Copy link
Collaborator Author

Any reason this hasn't been merged? 🤔

No one has reviewed it.

Copy link
Collaborator

@alborrajo alborrajo left a comment

Choose a reason for hiding this comment

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

I got plenty of comments regarding strings default value. By default they get a null value, which is probably never going to be a problem in this PR since the strings are always set to a different value, but its not uncommon to leave fields without setting during testing and having the server fail to serialize a null string, which is why its better to always initialize these values.

Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as in the other PR, it's better to have these in alphabetical order, as theyre already grouped by the group name prefix

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as in the banner PR, I don't think we should include copyrighted images in the repo

public ushort MaxSlotNum { get; set; }

public CDataGameItemStorageInfo()
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

GameItemStorage should be instanced in the constructor

public List<CDataGachaDrawGroupInfo> DrawGroups { get; set; }

public CDataGachaInfo()
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

The strings should be instanced in the constructor

public class CDataGPShopDisplayType
{
public uint ID { get; set; }
public string Name { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This string should be instanced in the constructor

ID = 1,
CourseID = 1,
Name = "Adventure Passport (active)",
ImageAddr = "http://localhost:52099/shop/img/payment/icon_course1.png",
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto regarding localhost

Copy link
Collaborator

Choose a reason for hiding this comment

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

more localhost

WeightDispTitle = "Percentage Provided",
WeightDispText = "Class S 100.0%\r\n*Probability of provision is rounded to one decimal place.",
ListAddr = "http://localhost:52099/shop/img/payment/icon_lot72.jpg",
ImageAddr = "http://localhost:52099/shop/img/payment/image_lot67_01_campaign2.jpg",
Copy link
Collaborator

Choose a reason for hiding this comment

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

more localhost

WeightDispType = 1,
FreeSpaceText = "",
ListAddr = "http://localhost:52099/sp_ingame/campaign/bnr/lotto/lot_icon_170316_03.jpg",
ImageAddr = "http://localhost:52099/sp_ingame/campaign/bnr/lotto/lot_icon_170316_03.jpg",
Copy link
Collaborator

Choose a reason for hiding this comment

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

more localhost

if (Server.WalletManager.RemoveFromWalletNtc(client, client.Character, walletType, request.Price))
{
// Failed to deduct the cost
return res;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldnt this set an error code?

@pacampbell
Copy link
Collaborator Author

I got plenty of comments regarding strings default value. By default they get a null value, which is probably never going to be a problem in this PR since the strings are always set to a different value, but its not uncommon to leave fields without setting during testing and having the server fail to serialize a null string, which is why its better to always initialize these values.

Thanks for the review. This was not actually my PR but I added the JSON config to it. Anyways, will go through and fix them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants