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

CP029 Image accessors with format information #131

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

steffenlarsen
Copy link

Adds an extension for more supported data types for image accessors and a new get_access overload on images that allows for image format information to be known by the accessor at compile-time.

@Ruyk Ruyk requested a review from AerialMantis July 2, 2020 20:21
};
template<> struct image_format_type<image_format::b8g8r8a8_unorm> {
using type = float4;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we maybe also define a helper alias:

Suggested change
};
};
template<image_format format>
using image_format_t = typename image_format_type<format>::type;

Copy link
Author

Choose a reason for hiding this comment

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

Good call. I have added this and changed the data type of the accessor returned by the new get_accessor to use this.

I'm not completely convinced that image_format_type is the best name for this struct, so I'm open to alternatives.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps image_access_type?

Copy link
Author

Choose a reason for hiding this comment

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

I think that's a good name, though I'm not fond of the _type suffix. I've renamed it to image_access. What do you think?

Signed-off-by: Steffen Larsen <[email protected]>
Copy link
Contributor

@AerialMantis AerialMantis left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor comments.

};
template<> struct image_format_type<image_format::b8g8r8a8_unorm> {
using type = float4;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps image_access_type?

the underlying image and the `image_format` given to `get_access` is one of the
combinations shown in the following table:

| `image_channel_type ` | `image_format` |
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be useful to specify the type of image_format_type_t for each value of image_format as well to save cross-referencing it.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think this would be the table for it, but I could add a new table with the mappings if that would be useful?

Signed-off-by: Steffen Larsen <[email protected]>
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