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

Some fix on definitions #155

Closed
wants to merge 0 commits into from
Closed

Some fix on definitions #155

wants to merge 0 commits into from

Conversation

nyaoouo
Copy link
Contributor

@nyaoouo nyaoouo commented Aug 29, 2023

No description provided.

Copy link
Contributor

@Supamiu Supamiu left a comment

Choose a reason for hiding this comment

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

Hi, thank you for contributing to SaintCoinach.

This PR comes with no context, 6.2 references in commit messages that are up to 8 months old, doesn't follow naming conventions and contains half the changes that are wrong, please make sure to add context to the PR using the PR's message and make sure your PR contains updates that are valid, because checking every single edit is really not fun.

SaintCoinach/Definitions/CharaCardDecoration.json Outdated Show resolved Hide resolved
@@ -30,9 +30,9 @@
}
},
{
"index": 18,
"index": 19,
Copy link
Contributor

Choose a reason for hiding this comment

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

Data shows that it's starting at 18 and repeats for 5, not 4 (I included 18 in the example below)
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for Deep Dungeon sheet
col 18: offset 28
col 19: offset 16
col 20: offset 17
col 21: offset 18
col 18 is used in
ffxiv_dx11.exe ver. 2023.07.26.0000.0000: 140A637E6
image
which is not a list of data but a type enum

Copy link
Contributor

Choose a reason for hiding this comment

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

Then what's col 18? Did you check against ingame data as you're in deep dungeon?

Copy link
Contributor Author

@nyaoouo nyaoouo Aug 30, 2023

Choose a reason for hiding this comment

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

Then what's col 18? Did you check against ingame data as you're in deep dungeon?

it seems a flag to identicate which type of sheet is for idx 19-22, just look at the full function, if this flag is 1, DeepDungeonMagicStone is used and when the flag is 2, DeepDungeonDemiclone is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really doing exe digging to update definitions, it's just based on what it's matching ingame as we compare data and UI.

Copy link
Contributor

@Supamiu Supamiu Aug 30, 2023

Choose a reason for hiding this comment

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

It means that you should implement it as a complexlink for col 18 then, better do it in this PR.

Here is an example of a complexlink:

"type": "complexlink",
"links": [
{
"when": {
"key": "Type",
"value": 2
},
"sheet": "Achievement"
},

SaintCoinach/Definitions/Materia.json Outdated Show resolved Hide resolved
SaintCoinach/Definitions/SpearfishingItem.json Outdated Show resolved Hide resolved
SaintCoinach/Definitions/Stain.json Outdated Show resolved Hide resolved
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.

2 participants