Allow to set min filter and mag filter for a texture in the j3m file #295

Closed
Nehon opened this Issue Jul 12, 2015 · 13 comments

Projects

None yet

4 participants

@Nehon
Contributor
Nehon commented Jul 12, 2015

Textures can have diffrent convolution algorithm when they are minified or magnified on screen.
There is a way to change this algorithm by code, throught the Texture API.

Though when you make a material though a j3m file, and want different filtering, you have to load your material by code and change the filtering in the code. It woudl be nice to be able to set the filtering directly in the j3m file the same we specify Flip or Repeat.

The keyworkd could be any value from the MinFilter and MagFilter enum foun din the Texture class
https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-core/src/main/java/com/jme3/texture/Texture.java

But each keyword should be prefixed with Min or Mag for example :
MinTrilinear MagBilinear.

@DannyJo
Member
DannyJo commented Jul 13, 2015

I've had a look at this one and it is pretty straight forward. Basically the implementation would split the "value" string using spaces and then parse each token to see if it should alter the texture/texturekey in any way (I call these Texture Options). The last token in the list would be the actual path to the texture.

However there is a potential for a defect here, a defect which I believe is already in place for the existing code as well for a specific use case.

When a texture has a path that contains spaces eg. DiffuseMap: Models/Sign Post/Sign Post.jpg this kind of parsing would not work and the derived path to the texture in this case would be "Post.jpg" instead of the full path. Now the existing code has a similar issue in that it does a string.startsWith check to see if the "path" starts with "Flip", "Flip Repeat" or "Repeat" and then performs a string.subString(index) to get the texture path. Now if the texture path actually was "Flip/SomeFolder/Texture.jpg" the derived path would become "/SomeFolder/Texture.jpg".

Good news is that this can now all be resolved with this added feature. When tokenising the path we would look for quoted strings as well, so for instance;

DiffuseMap: MinTrilinear MagBilinear Flip WrapRepeat "Models/Sign Post/Sign Post.jpg"

Would correctly parse out the different texture options as well as the correct texture path as it's escaped with quotes. The order of the Texture Options is also irrelevant, it will work either way.

Now the downside to this is that existing materials which use spaces in the path would stop working unless quotes were added. This could perhaps be prevented if we checked for the number of valid Texture Options in the value first and if there are no matches we assume the value is the full path.

@Nehon
Contributor
Nehon commented Jul 13, 2015

mhh sounds like a breaking change...
Maybe we could add a version to j3m files like we do with j3o.
This way, we'd consider j3m files with no version as in version 1.0 (meaning old parsing style even if it has flaws) and we could implement what you mention in version 1.1.

Anyway the current way of parsing this is a bit dumb, it needs to change, and you proposal looks fine to me.

@shadowislord @pspeed42 @normen what do you think?

@pspeed42
Contributor

Random things:
-if escaping is not already supported then that's kind of a problem
-we should only be accepting valid tokens anyway... so that would be one way to solve the problem
-paths (and all such user-defined strings) should probably have been quoted all along.

These are very common errors in hand-rolled parsers, though.

Anyway, to me the easy fix is to optionally allow quotes. That way we maintain backwards compatibility but also support quoted paths. I don't know how the split is done now but I'd think it's possible to adjust a regex to automatically catch quoted values in general. Might require some more general refactoring.

@DannyJo
Member
DannyJo commented Jul 14, 2015

At the moment there is no "split" as such, there are some very basic checks to determine what to do and only in a specific order (Flip Repeat) is supported. You can't declare it using "Repeat Flip" for instance.

Current code:

            if (texturePath.startsWith("Flip Repeat ")){
                texturePath = texturePath.substring(12).trim();
                flipY = true;
                repeat = true;
            }else if (texturePath.startsWith("Flip ")){
                texturePath = texturePath.substring(5).trim();
                flipY = true;
            }else if (texturePath.startsWith("Repeat ")){
                texturePath = texturePath.substring(7).trim();
                repeat = true;
            }

However I think I might have a solution which is backwards compatible now and that actually allows for much more if it detects any of the new options. It also supports quoted paths using the new and old way. I've also added support for the reverse order (Repeat Flip) when using the old way just for extra niceness.

Texture options would now include the following (order is irrelevant):

  • Min[Filter]
  • Mag[Filter]
  • Wrap[Mode]
  • Flip

Example combinations:

DiffuseMap: MinTrilinear MagBilinear WrapRepeat Flip "some/path/to a/texture.png"
DiffuseMap: Flip Repeat "some/path/to a/texture.png"
DiffuseMap: WrapEdgeClamp MagNearest MinNearestNearestMipMap "some/path/to a/texture.png"

I believe I could create a pull request from this now unless anyone has any questions or feedback regarding my logic.

As an added extra we could log a warning if the old way has been detected and tell the user that it has been deprecated and that they should use the new format. At least we're telling people to fix it but we're not removing functionality right now.

@afonsolage

What about instead of using space, which can conflict with paths, use comma?

DiffuseMap: MinTrilinear, MagBilinear, WrapRepeat, Flip, some/path/to a/texture.png
DiffuseMap: Flip, Repeat, some/path/to a/texture.png
DiffuseMap: WrapEdgeClamp, MagNearest, MinNearestNearestMipMap, some/path/to a/texture.png

Texture path will always be texturePath.split(',')[texturePath.length-1]

public class Main {

    public static void main(String[] args) {
        Main main = new Main();
        main.testParser("MinTrilinear, MagBilinear, WrapRepeat, Flip, some/path/to a/texture.png");
        main.testParser("Flip, Repeat, some/path/to a/texture.png");
        main.testParser("WrapEdgeClamp, MagNearest, MinNearestNearestMipMap, some/path/to a/texture.png");
        main.testParser("some/path/to a/texture.png");
    }

    private void testParser(String texturePath) {
    boolean minTrilinear = false;
    boolean magBilinear = false;
    boolean wrapRepeat = false;
    boolean flipY = false;
    boolean wrapEdgeClamp = false;
    boolean minNearestNearestMipMap = false;
    boolean magNearest = false;

    String[] keys = texturePath.split(",");

    for (String key : keys) {
        key = key.trim();
        if (key.equalsIgnoreCase("MinTrilinear")) {
            minTrilinear = true;
        } else if (key.equalsIgnoreCase("MagBilinear")) {
            magBilinear = true;
        } else if (key.equalsIgnoreCase("WrapRepeat")) {
            wrapRepeat = true;
        } else if (key.equalsIgnoreCase("Flip")) {
            flipY = true;
        } else if (key.equalsIgnoreCase("WrapEdgeClamp")) {
            wrapEdgeClamp = true;
        } else if (key.equalsIgnoreCase("MagNearest")) {
            magNearest = true;
        } else if (key.equalsIgnoreCase("MinNearestNearestMipMap")) {
            minNearestNearestMipMap = true;
        }
    }

    texturePath = keys[keys.length - 1].trim();

    System.out.println(String.format("Path: %s, minTrilienar: %s, magBilinear %s, "
        + "wrapRepeat: %s, flipY: %s, wrapEdgeClamp: %s, magNearest: %s, "
        + "minNearestNearestMipMap: %s", texturePath, minTrilinear, magBilinear,
        wrapRepeat, flipY, wrapEdgeClamp, magNearest, minNearestNearestMipMap));
    }
}

output:

Path: some/path/to a/texture.png, minTrilienar: true, magBilinear true, wrapRepeat: true, flipY: true, wrapEdgeClamp: false, magNearest: false, minNearestNearestMipMap: false
Path: some/path/to a/texture.png, minTrilienar: false, magBilinear false, wrapRepeat: false, flipY: true, wrapEdgeClamp: false, magNearest: false, minNearestNearestMipMap: false
Path: some/path/to a/texture.png, minTrilienar: false, magBilinear false, wrapRepeat: false, flipY: false, wrapEdgeClamp: true, magNearest: true, minNearestNearestMipMap: true
Path: some/path/to a/texture.png, minTrilienar: false, magBilinear false, wrapRepeat: false, flipY: false, wrapEdgeClamp: false, magNearest: false, minNearestNearestMipMap: false
@Nehon
Contributor
Nehon commented Jul 14, 2015

I'm not a fan of the coma separator. Also it also not backward compatible. IMO @DannyJo solution sound nice. All up for a PR !

@pspeed42
Contributor

Because that will ALSO break backwards compatibility in a big way, ie: every material ever everywhere becomes broken.

@pspeed42
Contributor

Yeah, I also look forward to seeing what @DannyJo has done... especially if it lets us also easily add things like repeat for just x or just y, etc.... because I had to abandon j3ms in Mythruna config for that reason.

@DannyJo
Member
DannyJo commented Jul 14, 2015

OK, that's fine I will send a PR this way as soon as I can.

@DannyJo
Member
DannyJo commented Jul 14, 2015

Not sure if this is what you're after @pspeed42 but I'm making a slight modification to this before I get it ready for a PR. It will now allow you to use the axis in setWrap() as well.

This line DiffuseMap: WrapRepeat_S "path/to/texture.png"

Translates to texture.setWrap(WrapAxis.S, WrapMode.Repeat);

@Nehon
Contributor
Nehon commented Jul 14, 2015

ok but also allow to an easy wrap around all axis like:
DiffuseMap: WrapRepeat "path/to/texture.png"
would wrap around S and T...
Also would ahve called them U and V... but it has to match with the WrapAxis enum....

@DannyJo
Member
DannyJo commented Jul 14, 2015

Exactly, the normal WrapRepeat will ignore the axis call. Actually any WrapXXX will ignore the axis, but you can also use WrapXXX_AXIS to take the axis into account.

We can always add more WrapAxis enums as aliases but that's outside the scope of this issue I believe.

@Nehon
Contributor
Nehon commented Jul 14, 2015

yep. Awesome.
Looking forward to this ;)
Thanks for stepping in.

@DannyJo DannyJo added a commit to Kendanware/jmonkeyengine that referenced this issue Jul 14, 2015
@DannyJo DannyJo Added support for setting minification and magnification filters on a…
… texture in the j3m material file. This also adds support for double and single quoted paths as well as being able to set WrapMode for a specific WrapAxis. This resolves #295
9059eb3
@DannyJo DannyJo added a commit to Kendanware/jmonkeyengine that referenced this issue Jul 20, 2015
@DannyJo DannyJo Added unit test for J3MLoader to cover the new texture parameters ava…
…ilable in #295. Also fixed a couple of issues in the code to reduce logging that was not needed and removed redundant code. This update also updates junit to 4.12 and adds Mockito and Fest Assertions as test dependencies.
d319a7c
@DannyJo DannyJo self-assigned this Sep 3, 2015
@Nehon Nehon closed this in #301 Sep 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment