Allow specifying texture parameters. about gearvrf

[soumyajitdeb]

We'll take this one up as it is important and requires immediate attention.

[lcbasu]

Hi @thomasflynn ,

We have done some implementation as a test for implementing texture enhancement parameters.

The change commit is here(https://github.com/SRIB-GRAPHICS/GearVRf/commit/8f0d6d66add2407247551913dd316ccc84661fc4) on the branch (https://github.com/SRIB-GRAPHICS/GearVRf/tree/texture-enhancement).

Please review the implementation approach so that we can proceed on similar lines.

Referencing other members for reviews. [ @soumyajitdeb @jason2kim ]

[thomasflynn]

did a quick skim of the code. some comments:

1. I don't see the point of creating enums for GL_LINEAR, GL_NEAREST, etc. Just allow the user to pass in GL_LINEAR, GL_NEAREST, etc. It'll get rid of all the switch() statements as well.
2. Is there any way to guarantee that getMaxAnisotropicValue() occurs on the GL thread? We should probably get this value and whether it is supported when we first setup the GL thread and then just return that stored information when it is asked for.
3. While the most common use case will be setting the texture parameters at texture creation time, we should also allow changing the texture parameters after the texture is created as well.

[lcbasu]

Hi @thomasflynn ,

We've taken care of the point (2) [https://github.com/SRIB-GRAPHICS/GearVRf/commit/1d8bf33e11360067abcf2eddf49cd47513886caa] & (3) [https://github.com/SRIB-GRAPHICS/GearVRf/commit/b8ac24e69008773f75ed1d2e5c69848f1bbec224] in the latest commit.

For point (1), we think that using enum along with switch is much cleaner as we tried using String for the first time and the code was not so clean once we were trying to pass values to JNI as there we will have to compare in the similar fashion. if you can please suggest some better ways than we will be happy to work on that.

These commits are just for getting the feel of how to proceed, once we are clear, we'll clean the code as per our regualr patters.

Thanks.

[phi-lira]

I did a quick skim and in general I think it's good. I just have a few points:

1. I think it would be better to have separate functions to set filter, wrap mode etc. If one wants to set filtering there's no need to reupload wrap state. This will avoid adding unnecessary driver overhead by issuing redundant api calls.

2. Regarding Tom's comment on enum, if you're making just a wrapper that calls same interface (GL_LINEAR, GL_NEAREST etc) you could pass in directly those values instead of using a switch;

One possible way of doing this is to declare enums in Texture class that maps directly to gl constants. Like follows:

enum TextureMinFilter : int {
    LINEAR = GL_LINEAR,
    NEAREST = GL_NEAREST,
    LINEAR_MIPMAP_LINEAR = GL_LINEAR_MIPMAP_LINEAR
    ...
};

enum TextureMagFilter : int {
    LINEAR = GL_LINEAR,
    NEAREST = GL_NEAREST,
};

And now your function can be:

setFiltering(TextureMinFilter min_filter, TextureMagFilter mag_filter) {
    min_filter_type_ = min_filter;
    mag_filter_type_ = mag_filter;

    glBind(...)
    glTexParameteri(target_, GL_TEXTURE_MIN_FILTER, (int)min_filter_type_);
    glTexParameteri(target_, GL_TEXTURE_MAG_FILTER, (int)mag_filter_type_);
    glBind(...)  
}

This is good for some reasons. First, it avoid the switch and the calling code doesn't need to include GL headers. Then it makes less error prone (client code won't be able to pass GL_LINEAR_MIPMAP_LINEAR to mag filter by mistake or mess up min/mag filter parameter order. Lastly, the cast from the enum to int will be safe since you declared enum as enum TextureMinFilter : int

3. Implementing a solution proposed in item2 also avoids the uncessary conversion from float to int inside your function. Not all compilers/platforms handle float to int conversion smoothly and we should do it with care. More info on: https://software.intel.com/en-us/articles/fast-floating-point-to-integer-conversions

[lcbasu]

Hi @phi-lira ,

Thank you for such an elaborate explanation.

We will surely start with this new suggestion.

Thanks

[lcbasu]

Hi,

We tried the suggested pattern on the Java side and, with some modifications we came to this [https://github.com/SRIB-GRAPHICS/GearVRf/commit/0337010fd7bb54296d3f13eb2aa4b2cf03a652d8] state.

Please give your input.

The conversion of float to int is still not taken care of. We are working on that.

Thanks.