Gamua / Starling-Framework

The Cross Platform Game Engine
http://www.starling-framework.org
Other
2.85k stars 819 forks source link

Request: Mesh.defaultStyleFactory #878

Closed tconkling closed 8 years ago

tconkling commented 8 years ago

Hey Daniel,

I have a custom shader that's encapsulated in a MeshEffect subclass and managed by a MeshStyle subclass. The shader requires a few params, which means that when I'm creating a new Image that uses it, I currently need to do something like this:

public static function createImage (texture :Texture, maskTex :Texture, overlayColor :uint) :Image {
    var prevStyle :Class = Mesh.defaultStyle;
    Mesh.defaultStyle = OverlayRecolorMeshStyle;
    var img :Image = new Image(texture);
    OverlayRecolorMeshStyle(img.style).setMaskAndColor(maskTex, overlayColor);
    Mesh.defaultStyle = prevStyle;
    return img;
}

This is fairly simple if I'm just creating a new Image, but if I'm creating a big display hierarchy through another library (Flump, for example), I need to traverse that entire hierarchy and update its params after creation.

Would you consider having a static "MeshStyleFactory" property on Mesh that would enable this to be done more simply? I'm thinking something along the lines of

public interface MeshStyleFactory {
    function createMeshStyle () :MeshStyle;
}

public class MeshStyleFromClass implements MeshStyleFactory {
    public function MeshStyleFromClass (meshClass :Class) {
        _meshClass = meshClass;
    }

    public function createMeshStyle () :MeshStyle {
        return new _meshClass();
    }

    private var _meshClass :Class;
}

The existing Mesh.defaultStyle getter and setter could be implemented in terms of MeshStyleFromClass (though the getter should probably be deprecated in favor of a Mesh.styleFactory).

If this is something you'd consider implementing, I'm happy to make a pull request!

PrimaryFeather commented 8 years ago

Hi Tim, first of all: sorry for my late reply!

when I'm creating a new Image that uses it, I currently need to do something like this [...]

What I'm not sure about here is why you're using Mesh.defaultStyle instead of just assigning the style to the mesh like this?

public static function createImage(texture:Texture, maskTex:Texture, overlayColor:uint):Image 
{
    var img:Image = new Image(texture);
    img.style = new OverlayRecolorMeshStyle(maskTex, overlayColor);
    return img;
}

Or do you just want to avoid the creation of the default MeshStyle by the Image constructor (via the Mesh parent class)?

As for the class factory: actually, I think this is not a bad idea! I'd definitely like to continue discussing this. Before we do that, though, I want to point you to one method that might do a similar trick; here's a method that's part of the MeshStyle base class:

/** Called when assigning a target mesh. Override to plug in class-specific logic. */
protected function onTargetAssigned(target:Mesh):void
{ }

That method is called when the style is assigned to a mesh. In this method, you could (depending on the mesh that is encountered) set up a few basic settings for it. For example, I'm using this in the LightStyle to update the texture coordinates for the normal map (which, per default, happen to be the same as those of the conventional texture).

Granted, this is not a solution for all cases, but I just wanted to be sure you're aware of this.

Maybe that would also be a good place to plug in some additional logic, by replacing / extending this with a callback. That way, you could e.g. create that OverlayRecolorMeshStyle independently (say, as part of a library) and then, at runtime (game code) assign its default settings based on the object it's assigned to.

tconkling commented 8 years ago

Do you just want to avoid the creation of the default MeshStyle by the Image constructor (via the Mesh parent class)?

Yes, exactly. I'm using Flump for animations, so I'm not actually creating the Images in game code - I'm calling out to the library, which returns a (large) display hierarchy - the characters in my game are pretty complex animations. I could traverse this hierarchy and manually reset each leaf node's MeshStyle to my desired subclass, but this seems pretty wasteful to me :)

Before we do that, though, I want to point you to one method that might do a similar trick; here's a method that's part of the MeshStyle base class:

I could definitely use onTargetAssigned to accomplish what I'm looking for, by e.g. storing the custom MeshStyle's params in some global somewhere, and then assigning them to the instance in the onTargetAssigned method (or just in my custom style's constructor, in fact). This feels like a slightly uglier workaround, though...

PrimaryFeather commented 8 years ago

I could traverse this hierarchy and manually reset each leaf node's MeshStyle to my desired subclass, but this seems pretty wasteful to me :)

Agreed! Thanks for the additional information.

This feels like a slightly uglier workaround, though...

Again, I agree. :wink: A factory, as you proposed, would definitely be a better solution for this issue. I'd maybe just simplify that a little bit. What about this?

Mesh.defaultStyleFactory = function(mesh:Mesh):MeshStyle
{
    if (mesh is FlumpImage)
    {
        return new OverlayRecolorMeshStyle();
    }
    else if (mesh.name.startsWith("flump-")) /* ... */
    else return null;
}

The mesh for which a style is needed is passed to that callback; that way, developers can insert their custom logic to decide which style makes sense for a given object, at a central place. If the method returns null, the standard Mesh.defaultStyle is used instead; so it would build on that functionality.

What do you say to that? Would that help / would you like that?

tconkling commented 8 years ago

Yeah, that'd work great!

(Out of curiosity, why a function pointer instead of an interface? Seems like the latter is more self-documenting.)

PrimaryFeather commented 8 years ago
Out of curiosity, why a function pointer instead of an interface? Seems like the latter is more self-documenting.

Both would work, I guess this is just a matter of personal preference. Since we're talking about just one method, I feel an interface is a little clumsy; users might have to write a class just to be able to implement it — assigning a function directly (even inline) looks like less effort to me. The same concept is used on several different places in the framework (e.g. the tween callbacks), so it's a good fit IMHO.

That said: it would be much better if AS3 would provide type-safety for methods. But well ... that won't happen. ;-)

In any case, I'll implement this change, then!

PrimaryFeather commented 8 years ago

Please give it a try and let me know if that works for you! And while we're at it, I'd be happy to hear any thoughts on this issue, from any readers of this thread. :smile:

JohnBlackburne commented 8 years ago

I've been reading. My solution has so far been to subclass Mesh, then add my own style & effect to that as I need it. I've yet to come across something that needs modifying in Starling, though I’ve poked around in the code trying to find out how things work (mostly PMA in Starling 1.7).

PrimaryFeather commented 8 years ago

Thanks for the comments, John! Yes, subclassing Mesh is a good idea in many cases. 👍

tconkling commented 8 years ago

Thanks Daniel. This works for me. Antihero is finally upgraded to Starling 2!

PrimaryFeather commented 8 years ago

That's great news, I'm happy to hear that. :smile: The game is looking fantastic, I can't wait to play it! Keep up the great work, Tim!