StarlingGraphics / Starling-Extension-Graphics

flash.display.Graphics style extension for the Starling Flash GPU rendering framework
https://github.com/StarlingGraphics/Starling-Extension-Graphics/wiki
MIT License
282 stars 89 forks source link

Texture no repeat in TextureVertexColorFragmentShader #140

Closed pSi-X closed 8 years ago

pSi-X commented 9 years ago

Hi, sory my bad English :) I encountered a problem that can be seen in the screenshot: Image of problem When I started to dig deeper, I found a bug in the class TextureVertexColorFragmentShader:

public function TextureVertexColorFragmentShader(texture:Texture = null, mipmapping:Boolean = false, repeat:Boolean = false, smoothing:String = "bilinear") {
   //Bad:
   var shouldRepeat:Boolean = texture == null ? repeat : ( texture.repeat ? repeat : false);
   //Good:
   var shouldRepeat:Boolean = texture == null ? repeat : texture.repeat;
   ...
}

It is a repeat of the texture will never be taken. Please fix this bug.

Best regards, pSi-X

IonSwitz commented 9 years ago

Thanks for reporting this, but.. if you set "texture.repeat=true" and the variable "repeat = true" when you call the constructor of TextureVertexColorFragmentShader, "shouldRepeat" will become true, yes?

I agree it's not a pretty solution, but it should at least give the correct functionality, I think?

pSi-X commented 9 years ago

Maybe that variant is correct:

var shouldRepeat:Boolean = texture == null ? repeat : (texture.repeat || repeat);

But yes, this solution repairs situation with TextureMaterial and TextureVertexColorFragmentShader :)

pSi-X commented 8 years ago

Please close the issue and to commit with such a decision:

var shouldRepeat:Boolean = texture == null ? repeat : (texture.repeat || repeat);

Thank you! :)

IonSwitz commented 8 years ago

This is a poorly designed API. The correct way to use this would be to force the person calling the method to specify the "repeat" argument to true or false. This is a good example where an argument with a default value causes problems.

To be honest, I don't want to change the API since that might cause backwards compatability issues. What situation do you have where you can't get the correct values to be set?

If you need the "shouldRepeat" to be set to either true or false, you can currently make that happen by setting values in the constructor appropriately, right?

If not, please show me a code example where things go wrong.

pSi-X commented 8 years ago

In the class TextureMaterial creates an instance of the class TextureVertexColorFragmentShader without repetition, and the default value false, so the value shouldRepeat var shouldRepeat:Boolean = texture == null ? repeat : ( texture.repeat ? repeat : false) always is false:

public function TextureMaterial(texture:Texture, color:uint = 0xFFFFFF, premultipliedAlpha:Boolean = true) {
    super(new StandardVertexShader(), new TextureVertexColorFragmentShader(texture));
    ...
}

And when I use the TextureMaterial, even if the texture.repeat == true, repeat value is taken by default, which is false.

P.s. Sorry for my english :)

IonSwitz commented 8 years ago

I have updated the TextureMaterial to take more arguments, that gets sent to the TextureVertexColorFragmentShader.

This should give you the flexibility you need without sacrificing backwards compatibility.

pSi-X commented 8 years ago

Thank you very much, now works as needed! ^_^

pSi-X commented 8 years ago

But now, an extra comma after texture,,:

super(new StandardVertexShader(), new TextureVertexColorFragmentShader(texture,, mipmapping, repeat, smoothing ));

https://github.com/StarlingGraphics/Starling-Extension-Graphics/issues/143