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
285 stars 88 forks source link

Interpolating thickness for lineTo/curveTo? #52

Closed IonSwitz closed 10 years ago

IonSwitz commented 11 years ago

Hey.

I've been coding graphics for two decades, but this is my first time developing in Starling and while digging around, I found this fantastic Graphics extension. Outstanding job, everyone!

However, as I try to use lineTo and curveTo, etc, I run into a problem. The thickness of the stroke is only controlled in the start point. Hence you get a line or curve with constant thickness between the control points.

I hacked some stuff into the code directly, to enable the use of an interpolating thickness ( allowing, for example, thickness 5 at the start and thickness 20 at the end), but my hack solution wasn't pretty. If I get a "Go Ahead" from anyone on here, I will clean up the code and submit a backwards compatible solution. What do you all say?

The solution would probably be along the line of adding a Function property to the Graphics class, with a signature like:

function thicknessFunction(startThickness:Number, endThickness:Number, lerp:Number) : Number

this would allow a user to add any kind of interpolating method (linear, sine, cubic) and with the property set to null (as would be the default) the code would be 100% backwards compatible.

Please let me know if this is something that might be desired.

EDIT: I should also mention that I am new to contributing to GitHub, so there might be some steps I am missing. What do I need to do to contribute? I'm sure there is a FAQ somewhere, as of now I can't find it. Probably my own fault ;)

makc commented 11 years ago

+1 nice feature even if it is not present in flash drawing api.

to contribute to single file, you just press "edit" button in its web page, edit or paste your changes in, and send pull request. if that's two files or more, you should use "fork" button, and then git client to clone repository to your computer, make changes there, push them back here to your fork and then send pull request.

robsilv commented 11 years ago

Hi @IonSwitz, your contributions and experience are most welcome, looking forward to seeing your updates!

Cheers Rob

IonSwitz commented 11 years ago

@makc: Should it be implemented in an external file, somehow then? How important is compliance to the Flash drawing API? Should I try to implement it in a way that it is very clear that this would be an extension to the API?

I could also implement it as a completely separate code path in the class, to make it even cleaner from that point of view. But it would cause some kind of nasty sections of duplicated code, basically duplicating the implementations of lineTo, curveTo and cubicCurveTo.

Let me know what you would prefer, I can go either way here.

makc commented 11 years ago

@IonSwitz I have nothing to do with this repo, btw. I am just watching it, that's how I was able reply to your post before everyone else )

IonSwitz commented 11 years ago

@makc Ok :) I'll implement it as completely separate code paths, that makes it easier to pull out if it is considered annoying or if it is getting in the way.

robsilv commented 11 years ago

@IonSwitz, the idea is to keep the core graphics API as similar to Flash as possible, in order to provide and easy on ramp for Flash devs, but there's no reason we can't add extra functions to that API which provide more functionality. There's also a lower level API which doesn't need to mimic Flash, more info here: https://github.com/StarlingGraphics/Starling-Extension-Graphics/wiki

On 10 Sep 2013 17:49, "IonSwitz" notifications@github.com wrote:

@makc Ok :) I'll implement it as completely separate code paths, that makes it easier to pull out if it is considered annoying or if it is getting in the way.

— Reply to this email directly or view it on GitHub.

IonSwitz commented 11 years ago

What I have now is code that allows for the following:

image

The leftmost curve is the "default" glowing lava example, with a thickness of 80. The middle curve is the same curveTo points (with an x offset) with a start thickness of 20, midpoint thickness of 120 and end point of 20 The rightmost curve is the same as the middle, but with the thickness numbers reversed: 120,20,120

The API has been extended with moveToEx, lineToEx, curveToEx, cubicCurveToEx, lineMaterialEx, lineStyleEx and lineTextureEx , leaving the regular API as is. The naming convention is stolen from the Windows COM world, which I really dont enjoy, but I am coming up short when it comes to good names here :)

I'm still not quite happy with the code design, but it is beginning to come together. Hopefully I can have something ready tomorrow evening.

makc commented 11 years ago

windows programmer detected :)

jonathanrpace commented 11 years ago

I would have chosen those names too :)

robsilv commented 11 years ago

Hi guys,

With the "Ex" naming convention, what do we do if another dev wants to extend these same functions with a different implementation? The logical thing then might be to enumerate the extensions, e.g. Ex1, Ex2, etc. This goes against the general principle of having descriptive function names though, where it's not obvious what the various implementations do without first reading the documentation.

I was thinking we could either: a) add optional parameters to the existing functions to support this functionality (which we possibly want to avoid to keep the basic API simple and similar to Flash) b) use more descriptive function names, e.g. lineToVarThickness(), or perhaps lineTo_varThickness() where the additional functionality (in this case "variable thickness") is spelled out.

My preference is "b)", as I prefer verbose names over obscure names.

Let me know your thoughts!

robsilv commented 11 years ago

Also, is there anything stopping us passing a number of thickness values as "thicknesses:Array", where the thicknesses are taken to represent the start and end values, plus a number of evenly distributed thicknesses along the path?

IonSwitz commented 11 years ago

After sleeping on this, I think the best way to go forward would be to have the Ex methods take a parameter object, instead of having Ex2, Ex3, etc.

Something like:

public class GraphicsExParameters { // Parameters to control thickness public var endThickness = -1; public var thicknessCallback:Function = null; public var thicknessArray:Array = null; // Parameters to control alpha along the line public var endAlpha:Number = NaN; public var alphaCallback:Function = null; public var alphaArray:Array = null;

// etc.. add more parameters to extend the functionality }

then the only extra parameter needed for the Ex-functions would be this parameter class, while keeping the back end of the API extensible.

I will implement this tonight, and if it turns out ok, I will (finally) start my GitHub fight to get the files committed. That is, unless someone finds this solution too cumbersome or ugly...

IonSwitz commented 11 years ago

@robsilv No , there is nothing that stops us from adding an array like that as well.I will see if I can add that tonight, along with the alpha stuff and the parameter class mentioned above

IonSwitz commented 11 years ago

Ok, on the third iteration, the code feels cleaner. I have done the following:

1) Changed "private"s to "protected"s in the Graphics class, to allow subclassing 2) Added the starling.display.graphicsEx namespace, containing GraphicsEx ( a sub class of Graphics, containing all the new code) ShapeEx ( a Shape that has a GraphicsEx as its "graphics" property) GraphicsExData ( a class containing the parameters mentioned above )

This makes the base class Graphics as clean as it has ever been, only more extensible And it adds a decent set of new abilities to the functionality, without intruding on the previous implementation.

I will try to commit the changes now. Wish me luck.

IonSwitz commented 11 years ago

Oh, and the thicknessArray / alphaArray code is not implemented. A few problems arose that we might discuss here:

If I submit an array of thickness points to curveToEx, what does the values in the array mean?

At what points along the curve should the thickness points in [ 30,60,40,80] be applied? Hard coded to 0, 0.33, 0.66 and 1.0 in the case of four entries? That doesnt seem to be very flexible, and not very pretty.

@robsilv Do you have any opinions on this? I'm wide open for a discussion, but I think an array like that is tricky.

An other alternative is to have a paired array, where each element contain two values: One Number saying where along the segment the value applies ( 0.0 - 1.0 value) and the other would be the actual thickness.

So, the sample array above would become: [[0,30],[0.33,60],[0.66,40],[1.0,80]]

It's not very pretty ;)

IonSwitz commented 11 years ago

Ok, I failed magnificently at getting my files added through git. I have worked with SourceSafe, ClearCase, TeamSystem, Perforce and SVN before, but Git kicks my ass and I am too tired to wrestle it tonight. ;)

If someone wants to try out the code, it is available in this ZIP archive (just the changed files are in here):

https://dl.dropboxusercontent.com/u/107200878/StarlingExtensionGraphicsEx.zip

If you want to have some simple sample code on how to use it, here's a piece based on the Flowing Lava sample:

var lavaThickness:Number = 90;

var background:Image = new Image(Texture.fromBitmap(new FinalBackgroundBMP())); background.width = stage.stageWidth; background.height = stage.stageHeight; addChild(background);

var shape:ShapeEx = new ShapeEx(); shape.blendMode = BlendMode.NORMAL;

addChild(shape);

var lavaMaterial:StandardMaterial = new StandardMaterial( ); lavaMaterial.vertexShader = new AnimateUVVertexShader( 0.1, 0 ); lavaMaterial.fragmentShader = new TextureVertexColorFragmentShader(); lavaMaterial.textures[0] = Texture.fromBitmap( new LavaTiledBMP(), false );

var param:GraphicsExData = new GraphicsExData();

var lavaThickness:Number = 10;

shape.graphics.lineMaterialEx( lavaThickness, lavaMaterial , 0.0); shape.graphics.moveTo( 150, 0 );

param.endThickness = 30; param.endAlpha = 1.0; param.thicknessCallback = firstStripThicknessCallback; param.alphaCallback = firstStripAlphaCallback;

shape.graphics.curveToEx( 500, 100, 500, 300, param );

param.endThickness = 10; param.endAlpha = 0.0; param.thicknessCallback = lastStripThicknessCallback; param.alphaCallback = lastStripAlphaCallback;

shape.graphics.curveToEx( 500, 500, 700, 650, param );

}

protected function firstStripThicknessCallback(from:Number, to:Number, lerp:Number) : Number { return (from * (1.0 - lerp) + (to * lerp)) + Math.random() * 10; } protected function lastStripThicknessCallback(from:Number, to:Number, lerp:Number) : Number { return (from * (1.0 - lerp) + (to * lerp)) + Math.random() * 20; }

protected function firstStripAlphaCallback(from:Number, to:Number, lerp:Number) : Number { return (from * (1.0 - lerp) + (to * lerp)) ; } protected function lastStripAlphaCallback(from:Number, to:Number, lerp:Number) : Number { return (from * (1.0 - lerp) + (to * lerp)); }

Enjoy. Feel free to try out the code and perform the commit if you thihnk it's good enough. I need to read up on Git before I try this again.

robsilv commented 11 years ago

Good point re the array approach and the arbitrary nature of where the thickness would be applied to the line. Perhaps it would be slightly neater if it were an Vector of specific objects, e.g. the class ThicknessPoint, which accepts thickness:Number and position:Number (the latter being a value between 0 and 1) in its constructor? Maybe there's a better name for the class and the properties, but would that make it a little better?

IonSwitz commented 11 years ago

It is definitely something to consider. I'm in Sweden, it is late-ish, and Ive had a few After Work beers, so I will sleep on this and, if I get some time this weekend, I will look over it.

In general, I am happy with the solution to keep the original Graphics API untouched, making the Graphics base class extendable (moving all "privates" to "protecteds") , and adding things to the separate sub-namespace GraphicsEx. I will try to come up with some fun sample code as well, if desired.

But, I'd rather nail down the code design before start writing sample code targeting an API in flux.

IonSwitz commented 11 years ago

I am beginning to reconsider this move. The code gets more and more nasty the more I want to add to the code.

I am beginning to think that post-processing is a better way to go. Basically, keeping the regular Graphics API, extending it with the natural spline code, and then add a second pass over the generated Vector of StrokeVertices, to add thickness, alpha, even vertex color to the StrokeVertex-vector afterwards.

There is really very little to be gained from being able to set these factors in in the same API call.

robsilv commented 11 years ago

Sounds like good thinking to me, so long as it doesn't add a big overhead?

IonSwitz commented 11 years ago

There is the issue of having to run through the StrokeVertex twice, once for creating the regular line with the regular API (or the natural Spline API) and then once more with the post processing, setting thickness, alpha and color.

However, if we do everything at one run, the inner loops gets filled with "if"s, which tend to cause performance issues as well.

I haven't really done any heavy performance testing on either scenario, but the post processing makes it a lot easier to, for example, add alpha to the start and the end of the line, without having to process every vertex inbetween.

So I think that the post processing will be faster in most reasonable scenarios, and negligible in the "postprocess every StrokeVertex" scenario.

IonSwitz commented 10 years ago

Closing this, the post processing is now part of the GraphicsEx API