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

[Cata] tag wotlk S5 honor weapons as never available #1886

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

ZeChrales
Copy link
Contributor

tested on cata classic
on retail, items are not flagged correctly because they were added in 7.x into the weapon arsenal, I suppose some specific flag should be added

@ZeChrales ZeChrales changed the title tag wotlk S5 honor weapons as never available [Cata] tag wotlk S5 honor weapons as never available Jan 19, 2025
Copy link
Member

@NORPG NORPG left a comment

Choose a reason for hiding this comment

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

To avoid merge conflicts, you can upload only the modified source files, and the file in the db folder will be automatically generated when releasing.

db/Cata/Categories.lua Outdated Show resolved Hide resolved
@DFortun81
Copy link
Member

For pull requests I'd advise not including Categories or LocalizationDB since those files tend to have a lot of merge conflicts. Also for these items, you have the CREATED timestamp, but not when it was ADDED to the game. These appearances should be available in Retail. Just gotta figure out which build to use.

@ZeChrales
Copy link
Contributor Author

For pull requests I'd advise not including Categories or LocalizationDB since those files tend to have a lot of merge conflicts. Also for these items, you have the CREATED timestamp, but not when it was ADDED to the game. These appearances should be available in Retail. Just gotta figure out which build to use.

It was never added, even in retail. You can only get them with arsenal (which is correctly done already). You think I need to add the ADDED version from this arsenal addition ?

@ZeChrales ZeChrales force-pushed the clean_cata_savage_pvp branch from f2a1612 to 577806c Compare January 19, 2025 17:30
@DFortun81
Copy link
Member

DFortun81 commented Jan 19, 2025

For pull requests I'd advise not including Categories or LocalizationDB since those files tend to have a lot of merge conflicts. Also for these items, you have the CREATED timestamp, but not when it was ADDED to the game. These appearances should be available in Retail. Just gotta figure out which build to use.

It was never added, even in retail. You can only get them with arsenal (which is correctly done already). You think I need to add the ADDED version from this arsenal addition ?

I'm not entirely sure. It looks like the arsenal itself uses a SYMLINK to this header to fill its contents. Since it was added in 7.2.0 according to its timeline, it should be appropriate to add that timeline to the weapons after their CREATED timeline entry.

If we don't, those items will be marked as NYI in retail as well since SYM links only refer to external sources, not infer and replace timeline values for those sources.

@ZeChrales
Copy link
Contributor Author

Done, but I don't see any diff in retail tooltips !

@@ -101,108 +101,108 @@ root(ROOTS.PVP, applyclassicphase(WRATH_PHASE_ONE, run(MarkOfWHOOOWHATNow, bubbl
}),
-- Savage Gladiator weapons have never been implemented
i(42557, { -- Savage Gladiator's Barrier
["timeline"] = { CREATED_3_0_2 },
["timeline"] = { CREATED_3_0_2, ADDED_7_2_0 },
Copy link
Member

Choose a reason for hiding this comment

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

@ImUnicke Could you please help clarify, when "created" and "added" exist at the same time, will NYI be applied when the created timeline is true and the added timeline is false, and will NYI not be applied when the created timeline is true and the added timeline is true?

Copy link
Member

@NORPG NORPG Jan 22, 2025

Choose a reason for hiding this comment

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

截圖 2025-01-22 下午5 04 51

I tested it in a retail environment, the version was 11.0.7, which met the created and added conditions, but it was still marked as NYI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tested it in a retail environment, the version was 11.0.7, which met the created and added conditions, but it was still marked as NYI.

It wasn't reparsed for retail with pull preview. After it will look fine -
image

Copy link
Member

Choose a reason for hiding this comment

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

It wasn't reparsed for retail with pull preview. After it will look fine - image

okay, maybe it's an environment problem on my side, I didn't change anything after parser :( .

@NORPG NORPG merged commit f767136 into ATTWoWAddon:master Jan 22, 2025
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.

4 participants