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

Use item id instead of name in monster loot #3538

Closed
wants to merge 2 commits into from

Conversation

marmichalski
Copy link
Contributor

Pull Request Prelude

Changes Proposed

Issues addressed: #3528

@marmichalski
Copy link
Contributor Author

If you prefer, I can keep (& add missing) comments with item name 🤔

@Zbizu
Copy link
Contributor

Zbizu commented Aug 3, 2021

Why did you remove the comments on item ids? It tells exactly what the user is editing and makes the whole thing readable.

If you're using a script to convert that, call ItemType(itemId):getName() on each item id to generate comments again.

@marmichalski
Copy link
Contributor Author

marmichalski commented Aug 3, 2021

Why did you remove the comments on item ids? It tells exactly what the user is editing and makes the whole thing readable.

#3538 (comment)

Keeping the comment here also makes it easily outdated when items.xml are changed. There were already a few missing names used.

If you're using a script to convert that, call ItemType(itemId):getName() on each item id to generate comments again.

Do you just type things for the heck of typing?

@soul4soul
Copy link
Contributor

I'd rather have the item names and run the risk of duplicate item name. At a minimum the item names have to be kept as comments to make editing the files manageable.

@marmichalski
Copy link
Contributor Author

Will this be stalled until merge conflicts arise?

@DSpeichert DSpeichert added the decisions Debatable/disputable label Aug 20, 2021
@Erza
Copy link
Contributor

Erza commented Aug 20, 2021

To help with your decision: id's should be used because of name duplication - you can't reliably use item names as loot because multiple items can have the same name and the engine will pick the first id that it finds.

Now when you look at #3540, non-unique item names use id's instead, but for one, that's inconsistent, and for two, nobody wants to maintain that. It's not future-proof, if item names ever get corrected and one ends up with a duplicate name

Just use id's across the board and merge this PR.

@marmichalski
Copy link
Contributor Author

@EPuncker beep boop

@Erza
Copy link
Contributor

Erza commented Sep 3, 2021

smh

@EPuncker
Copy link
Contributor

EPuncker commented Sep 3, 2021

@DSpeichert this can be merged, I can't approve it because my browser lag with so many files

@DSpeichert
Copy link
Member

I think someone suggested keeping both id and name parameter, instead of moving the name out to a comment. It's easier to write and it's also more portable. IIRC, the engine will prefer id, if exists, before reverting to name.

@marmichalski
Copy link
Contributor Author

kthxbye

@marmichalski marmichalski deleted the monsters-loot branch September 3, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decisions Debatable/disputable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants