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 collectibles for displayProperties on redacted items #3427

Merged
merged 4 commits into from
Dec 11, 2018

Conversation

joshhunt
Copy link
Contributor

@joshhunt joshhunt commented Dec 9, 2018

Most redacted items have Collectibles that are not redacted. This PR uses them for displayProperties of name, icon and description where they can be used. It still doesn't let us equip the item or determine its power, but at least the item is now searchable and provides a bit more context.

I also had to make a change to LazyDefinition to support getting all definitions of a type (because I need to key Collectibles by itemHash). I hope this is okay and I've done it correctly 😇

screenshot 2018-12-09 at 13 34 15

@bhollis
Copy link
Contributor

bhollis commented Dec 10, 2018

@joshhunt I tweaked it a bit, mostly just so we don't load everything unless we have to, and so we keep track of what we've loaded so subsequent get calls don't need to reload what's already been loaded. Would you mind trying it out and making sure it works before I merge, since I don't have any redacted items?

@delphiactual
Copy link
Contributor

delphiactual commented Dec 10, 2018

We may also want to do something to the tile besides the ??? to highlight that this item is classified at a glance.

Edit:
Especially if Bungie-net/api#138 ever sees the light of day

@joshhunt
Copy link
Contributor Author

@bhollis I've updated it a bit and given in a whirl and everything looks good from here...

@bhollis bhollis merged commit 7f59183 into DestinyItemManager:master Dec 11, 2018
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