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.

Sign up for GitHub

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

Jump to bottom

[Feature Request] Consider adding enumerations for glTF sampler filters and wrap modes #256

Open
matthew-rister opened this issue Oct 27, 2024 · 5 comments · May be fixed by #258
Open

[Feature Request] Consider adding enumerations for glTF sampler filters and wrap modes #256

matthew-rister opened this issue Oct 27, 2024 · 5 comments · May be fixed by #258

Comments

@matthew-rister
Copy link

matthew-rister commented Oct 27, 2024 •
edited
Loading

cgltf_sampler defines sampler filter and wrap modes using cgltf_int. This is unfortunate because it requires referring to the glTF specification to understand what values are allowed for each property in addition to hardcoding those values into a glTF loading implementation. A better user experience would be to represent those values using a dedicated enum similar to other cgltf enumerations such as cgltf_primitive_type and cgltf_attribute_type. A fix for this should be relatively straightforward and would look something like

// filter and wrap mode values defined at https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#reference-sampler
typedef enum cgltf_filter_type {
  cgltf_filter_type_nearest = 9728,
  cgltf_filter_type_linear = 9729,
  cgltf_filter_type_nearest_mipmap_nearest = 9984,
  cgltf_filter_type_linear_mipmap_nearest = 9985,
  cgltf_filter_type_nearest_mipmap_linear = 9986,
  cgltf_filter_type_linear_mipmap_linear = 9987
} cgltf_filter_type;

typedef enum cgltf_wrap_type { 
  cgltf_wrap_type_clamp_to_edge = 33071, 
  cgltf_wrap_type_mirrored_repeat = 33648, 
  cgltf_wrap_type_repeat = 10497,
  cgltf_wrap_type_default = cgltf_wrap_type_repeat
} cgltf_wrap_type;
@matthew-rister matthew-rister changed the title [Feature Request] Consider adding enumerations for glTF sampler filters and wrap modes. [Feature Request] Consider adding enumerations for glTF sampler filters and wrap modes Oct 27, 2024
@zeux
Copy link
Contributor

zeux commented Nov 3, 2024

Makes sense to me, it's the only case in the current API where integers are used instead of enums. Mind contributing a PR?

@matthew-rister
Copy link
Author

Gladly! Before I proceed, I would just like to confirm that you’re in alignment with the proposed approach of assigning glTF filter / wrap modes as enumeration values to maintain backward compatibility with implementations that previously relied on cgltf_int? This would diverge from the way that other enumerations are handled within the library, although the end-user experience should remain the same.

@zeux
Copy link
Contributor

zeux commented Nov 3, 2024

Hmm yeah I see. It's... probably fine? Any change to enum would require code that assigns these to change (but I'd expect this to be less frequent, as it only impacts cgltf_write). Using sequential enum values would silently break code like this:

https://github.com/google/filament/blob/main/libs/gltfio/src/GltfEnums.h#L27-L75

... whereas using enum with GL values would keep the code above working, and any issues would manifest as compile time errors, which feels safer. Ideally this probably should have used sequential enums to begin with, but right now changing this to non-sequential enums is the safest change. If this becomes a problem maybe these can be renumbered down the line (which would not break any users who migrate to using enum values).

@zeux
Copy link
Contributor

zeux commented Nov 3, 2024 •
edited
Loading

Similar code from gltfpack: https://github.com/zeux/meshoptimizer/blob/master/gltf/write.cpp#L794-L820 -- this will also work without any changes if an enum with non-sequential values is introduced.

Minor note: cgltf_filter_type should also have an entry cgltf_filter_type_default = 0 (or whichever other name you think fits). This is because the specification does not specify default values for minFilter or magFilter, and when these aren't specified it allows the application to choose:

Client implementations SHOULD follow specified filtering modes. When the latter are undefined, client implementations MAY set their own default texture filtering settings.

@matthew-rister matthew-rister linked a pull request Nov 15, 2024 that will close this issue
Open
@matthew-rister
Copy link
Author

Thanks for your patience with getting this out. Here is the PR with the proposed changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add glTF sampler filters and wrap modes enums matthew-rister/cgltf
2 participants
@zeux @matthew-rister