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

fix(#957): support iconhash/image data variant fields #965

Merged
merged 6 commits into from
Oct 25, 2023

Conversation

Mishura4
Copy link
Member

@Mishura4 Mishura4 commented Oct 21, 2023

WIP fix for #957
Do not squash

Code change checklist

  • I have ensured that all methods and functions are fully documented using doxygen style comments.
  • My code follows the coding style guide.
  • I tested that my change works before raising the PR.
  • I have ensured that I did not break any existing API calls.
  • I have not built my pull request using AI, a static analysis tool or similar without any human oversight.

@Mishura4 Mishura4 added bug Something isn't working enhancement New feature or request code Improvements or additions to code. labels Oct 21, 2023
@Mishura4 Mishura4 self-assigned this Oct 21, 2023
@netlify
Copy link

netlify bot commented Oct 21, 2023

Deploy Preview for dpp-dev ready!

Name Link
🔨 Latest commit 7ba6045
🔍 Latest deploy log https://app.netlify.com/sites/dpp-dev/deploys/65384bcd855b8100088b62a8
😎 Deploy Preview https://deploy-preview-965--dpp-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 21, 2023
@Mishura4 Mishura4 linked an issue Oct 21, 2023 that may be closed by this pull request
@Mishura4 Mishura4 removed the documentation Improvements or additions to documentation label Oct 21, 2023
@braindigitalis
Copy link
Contributor

my only concern with this change is that it changes ALL iconhash, giving them ALL this new upload method, when only ONE icon in the entire of discord api supports this approach (guild banner). This seems to then portray to the user they can upload any icon this way (bot pfp, guild icon, etc) when they cant. There are also other api endpoints that support different methods of uploads (message creation, webhook creation - both these are massively different in the way they work with webhook being closest to what guild banner does here).
Do we have a way in place to prevent the upload functionality working in places where it should not?
Also is the variant smaller, or larger than the iconhash? I dont know the internals of variant, isnt it as big as its largest possible type in the template?

@braindigitalis
Copy link
Contributor

braindigitalis commented Oct 22, 2023

Looking into the way theoretically a variant might work, the size of the icon type will be no larger than iconhash, because sizeof(iconhash) would be larger than sizeof(image_data). Therefore the only question remains is: how do we make it so that it doesnt seem to users like they could use the upload methods on a user pfp or something? Can we do something with a template and enable_if to directly flag uploadable fields and hide the methods elsewhere?

@Mishura4
Copy link
Member Author

Mishura4 commented Oct 22, 2023

No this doesn't change all iconhash. Iconhash is unchanged.

  • utility::iconhash: same as before. 16 bytes
  • utility::image_data: ptr to std::byte[], size, dpp::image_type. 16 bytes (size is uint32_t to save space)
  • utility::icon: wrapper around variant<icon_hash, image_data> with convenience methods (bool is_hash() etc). 24 bytes

Not sure yet if i need to replace all iconhash fields wirh icon. So far it's only the guild fields.
The variant does add some bytes for the index in the variant. Sadly no way around that unless we want to bet one part of the hash is never 0 or something like that (as discussed in Discord)
A bot can change their own pfp, this is something I'd like to support.

@Mishura4 Mishura4 marked this pull request as ready for review October 24, 2023 22:52
@Mishura4
Copy link
Member Author

(please do not squash merge - many changes! 📈)

@braindigitalis
Copy link
Contributor

When this is merged to dev, which examples in the dpp docs will need to be changed?

@Jaskowicz1
Copy link
Contributor

When this is merged to dev, which examples in the dpp docs will need to be changed?

From how this PR looks, no docs need to be changed.

@braindigitalis braindigitalis merged commit c90d17a into brainboxdotcc:dev Oct 25, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working code Improvements or additions to code. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

D++ does not support setting images (e.g) banner through guild_edit
3 participants