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

Vendor Lord Saladin's manifest definition has missing info #696

Closed
serkanbal opened this issue Sep 21, 2018 · 15 comments
Closed

Vendor Lord Saladin's manifest definition has missing info #696

serkanbal opened this issue Sep 21, 2018 · 15 comments
Labels
bug filed A bug has been filed in BNet's internal bug tracking system for this request/report. bug ready for release

Comments

@serkanbal
Copy link

Lord Saladin (vendorHash: 895295461) manifest definition has a missing icon (/img/misc/missing_icon_d2.png) for displayProperties.largeIcon property. All other vendors have a proper image for this field.

I use these images in my implementation of vendors; so it would be awesome if we can have a proper image url for this field in a future manifest update.

@vthornheart-bng
Copy link
Contributor

Mmm, good eye! I'll take a look into that.

@vthornheart-bng vthornheart-bng added the bug filed A bug has been filed in BNet's internal bug tracking system for this request/report. label Sep 25, 2018
@vthornheart-bng
Copy link
Contributor

TFS 715366

@serkanbal
Copy link
Author

Hello,
Just wanted to remind that this issue still persists in the latest manifest.
Thanks.

@justrealmilk
Copy link

TFS means it's been to do listed! maybe 11/27 (next week)

@vthornheart-bng
Copy link
Contributor

Ah, my apologies for not responding earlier! The bug is filed but I have not had a chance to follow up on it yet.

Just as a heads up on TFS issues in general - our backlog is absolutely massive (both in internal, non-user-facing work and external-facing work, both sourced from here and from our own feature work), and I'm the only person working on the Destiny API with any consistency (we are training up someone to work on the stats side which is a great relief). Filing it in TFS allows us to make sure it doesn't get lost entirely, but there's going to be a long turnaround time for lower priority issues as a result.

This isn't going to be in the 11/27 release as a result. I'll keep you posted here when I do get a chance to figure out what's going on, which should hopefully be today - and would make it go live for the first BNet release in December!

@vthornheart-bng
Copy link
Contributor

Okay, so not great news so far. I took a look and the game content itself doesn't have that data, which is why we don't have it. I'm filing a bug with the game side to get more info - but if they come back to me with something like "Vendors don't have to have this image, we aren't fixing it" (which I strongly suspect they're going to respond with based on the fact that he's gone this long without a large icon), then I can do something janky like create a fake one from his location image. But it's basically just going to be an appropriately cropped version of that image, rather than one that's "game canonical". At the moment, there is no game canonical version of this image, and my strong suspicion and fear is that there won't ever be one and we can't guarantee in the future that vendors will have one.

@vthornheart-bng
Copy link
Contributor

Screw it, I'm not going to wait for a response - even if they do add it, it'll be a couple of months before we see it. I'm going to go ahead and add a manual override on our side and give you a "non-canonical, but hopefully similar enough to be useful" image cropped from the larger background image that we provide. I'll see that it comes through in the December release!

@vthornheart-bng
Copy link
Contributor

image

How's that?

@vthornheart-bng
Copy link
Contributor

There we go, fixed up for December - as long as this workaround works for you!

@serkanbal
Copy link
Author

It looks great!
Thanks @vthornheart-bng and sorry for the extra work. I know you have a lot on your plate and so little time.

I will modify my code to handle the situation where the vendor api data may not return the mentioned field.

@vthornheart-bng
Copy link
Contributor

No worries - I wish I had gotten around to it sooner! I appreciate when you all let me know about this stuff, and I wish our turnaround time was better. :(

Since that image works for you, we could make that a standard going forward: If they don't provide these images, we can crop them out of the background one. That'll do the trick! Sweet!

@lowlines
Copy link

lowlines commented Dec 1, 2018

Hmm it looks like Tyra Karn [1748437699] is without a banner image too.
[edit]
Also the Emissary [3190557734] as well.

@vthornheart-bng
Copy link
Contributor

Fixed as of v2.3.3 deployment (12/11)!

@vthornheart-bng
Copy link
Contributor

Ugh, it has come to my attention that this fix didn't actually make it live. It's in a different branch that won't be live until January. Sorry for the false alarm!

@vthornheart-bng
Copy link
Contributor

This should be fixed as of today's deployment (1/29)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug filed A bug has been filed in BNet's internal bug tracking system for this request/report. bug ready for release
Projects
None yet
Development

No branches or pull requests

4 participants