Suggestions for a better cocos2d-runtime

Etabubu

Hi,

I bought Spine few days ago and it's a great editor. Very high quality.
I can't say the same for the cocos2d-runtime though.
I know the runtimes are still under heavy development and that's why I'd like to suggest a few points to improve the cocos2d one.

I'm an experienced Objective-C and Cocos2d programmer (4+ years) and I know very well how long it takes to get used to Objective-C and its design patterns. Especially if you come from C/C++, you tend to write classes the "C++ way" which is suboptimal in the iOS/OS X world.

So, please, don't take any offense with this post, I'm just trying to "guide" you towards a cleaner and more useful interface for the cocos2d-runtime. I'm not going to talk about the internal implementation of Spine-c because that's not my business.

Points I think you should really consider for the next refactoring:

Both class and instance methods need to follow Apple guidelines in regard to naming conventions
Class methods that return instances (basically factory methods) should always return "id" and not a strong type (all the "create:" methods in this case). If you don't do that, then it's very hard to subclass your class
All relevant data that is supposed to be accessed should be exposed through properties. This is very important: doing something like _skeleton->state->time in ObjC is really really bad. It breaks encapsulation and a thousand other conventions. You should have a @property (nonatomic, readwrite/readonly) for each of this data and return ObjC types and not C types (don't return const char *, but return NSString *)
Delegation pattern: it's probably the main design pattern used on iOS/OS X and every ObjC programmer is expecting this in a class like CCSkeleton. I'd like to be notified when an animation ends or when it loops over or when an event occurs... all these things must be implemented through a delegation pattern
Expose the main features of the spine-c in the CCSkeleton interface. If there's a setAnimation method, there should also be a stopAnimation

Those are my main suggestions.
I understand you're writing many runtimes at the same time and you're probably overwhelmed, but I strongly disagree with you when you say that writing a complete ObjC wrapper is not worth it.
I think it's exactly the opposite: you have a professional tool like Spine which is supported by a poorly written runtime (at least the Cocos2d one), and this doesn't make any sense at all. Games require a lot of features when handling animations (stop/pause/events/blending...) and these must be implemented in the runtime.

Thanks for reading this long post!

Marco

Lazrhog

I have ported the entire runtime into obj-c, but it is not completely compatible with the existing runtime, as I am using TexturePacker and gles2.0 custom shaders. It's not quite finished yet, but you can see my ios thread on here somewhere that details my progress

Etabubu

Oh cool, I'll take a look. Thx!

Nate

Thanks for the feedback. 🙂 You seem to be expecting a full blown ObjC API, but obviously this is not provided. CCSkeleton is an ObjC scene graph node for cocos2d. It does the rendering needed for cocos2d using data objects from spine-c, a C API. CCSkeleton provides some methods for basic usage, basically those for Skeleton and AnimationState, but there is no ObjC wrapper for spine-c. The spine-c API should be used directly.

The spine-c object graph contains lists, references to other objects in the graph, etc. This can't be wrapped this easily, converting lists to NSMutableArray, wrapping structs in ObjC classes, etc because then the rest of the spine-c code could not interpret the data. Attempting to keep two parallel object graphs in sync would be nasty. What you would end up doing is rewriting the entire runtime, duplicating all the spine-c data structures and logic in ObjC. For what purpose? So you can do the exact same job you could have done in C, but with a different syntax? I don't believe that is worth duplicating the codebase, increasing the cost of maintenance and future development. I understand ObjC developers prefer to use ObjC, however it should not be an issue to use a C API.

Etabubu wrote
- Both class and instance methods need to follow Apple guidelines in regard to naming conventions

Please be more specific with your proposed changes and I will consider them.

- Class methods that return instances (basically factory methods) should always return "id" and not a strong type (all the "create:" methods in this case). If you don't do that, then it's very hard to subclass your class

Good point, thanks.

- All relevant data that is supposed to be accessed should be exposed through properties. This is very important: doing something like _skeleton->state->time in ObjC is really really bad. It breaks encapsulation and a thousand other conventions. You should have a @property (nonatomic, readwrite/readonly) for each of this data and return ObjC types and not C types (don't return const char *, but return NSString *)

There are plenty of good reasons to use properties, but I don't believe in a blanket rule that everything should be properties. The pointers are to C structs, not ObjC classes, so memory management is not an issue. The spine-c API is C, so obviously ObjC properties don't apply there.

- Delegation pattern: it's probably the main design pattern used on iOS/OS X and every ObjC programmer is expecting this in a class like CCSkeleton. I'd like to be notified when an animation ends or when it loops over or when an event occurs... all these things must be implemented through a delegation pattern

The runtime can be used in many ways and does not try to solve everyone's application specific problems. Instead it provides an API that makes it trivial to solve those problems. However, if we were to identify additional utility common enough to be included in the runtimes, we could include it. Eg, blending is tricky so AnimationState was added to help manage it. Using AnimationState is completely optional.

You seem to want a higher level API for cocos2d. That would be fine, as long as it doesn't restrict how the API is used. Eg, an event callback might not make sense if you are doing procedural animation or applying 2 animations and mixing a 3rd. We should talk more about use cases and how the current API would handle them, then we can see more clearly what can be done to handle them better.

- Expose the main features of the spine-c in the CCSkeleton interface. If there's a setAnimation method, there should also be a stopAnimation

AnimationState does not have an explicit method to stop animating. setAnimation(null) can be used. I will add a clearAnimation method though, as it is generally less nice to overload a method parameter to use null as a special case.

I hope you can better appreciate the issues at hand. Claiming things are "poorly written" when you don't understand them or don't agree with them is not constructive and is not useful for gaining leverage to get what you want. I am not easily offended and not offended now, I just want to make it clear we can have a nice discussion without this criticism nonsense. 🙂 I am open to changes and will make them if I can be shown there are good reasons to make them. You would be better served focusing your efforts on showing why your proposed changes are so good. I like discussing my code and I'm all for improving things! 🙂

Etabubu

Thanks for taking the time to read and reply to my post. I know you're superbusy!
I'm usually very direct when talking about these things, so, please don't take it personally if it sounds like I'm "attacking" you. 🙂

I hope you can better appreciate the issues at hand. Claiming things are "poorly written" when you don't understand them or don't agree with them is not constructive and is not useful for gaining leverage to get what you want. I am not easily offended and not offended now, I just want to make it clear we can have a nice discussion without this criticism nonsense.

Criticism nonsense? I actually suggested you how to improve the API with a list of points and reasons. That's called constructive criticism...
When I said "poorly written" I wasn't talking about the spine-c API or its internal design, but I was referring to the ObjC interface, which is, in fact, "poorly written". I'm not saying this to offend you (why would I do that?), but to quickly make you understand that this API, compared to other free/commercial APIs out there (cocos2d, Chipmunk, ObjectAL, ... and some of those have a C core and just a ObjC wrapper, like Spine) is of a lower quality.
It "screams" C/C++, and that's my main complain.
I'm a seasoned C++ programmer as well, and I love it, but not in a ObjC context. And you learned this as well, when you refactored it to avoid having to rename all the files .mm just to make it compile.

I'm not really concerned about the integration with Cocos2D, I'm more worried about the ObjC correctness and proper use of the language's fantastic features (properties, delegation, protocols...) that you're very skeptical on implementing.
Of course, I'm not so naive to ask you to add features that only I would need. That's not the point. If you think that a delegation pattern to inform when an animation ends is not needed by anyone, then I won't complain (but I just read another post where someone was asking a similar thing and you decided to implement the "isComplete" flag to the C structure).

The spine-c object graph contains lists, references to other objects in the graph, etc. This can't be wrapped this easily, converting lists to NSMutableArray, wrapping structs in ObjC classes, etc because then the rest of the spine-c code could not interpret the data. Attempting to keep two parallel object graphs in sync would be nasty. What you would end up doing is rewriting the entire runtime, duplicating all the spine-c data structures and logic in ObjC.

No, you don't need to go that way of course. I'm not asking that! 🙂
But for example, if I want to retrieve the current animation time, or duration, or name... or any other field in that structure (and other structures you're using), I should be able to access them through properties. Even if they're just readonly properties.
This is extremely important in an API because tomorrow you might change the AnimationState struct in the Spine-C for some good reason, and my code won't compile anymore. I'll have to go through all my sources where I do _skeleton->state->animation->duration and fix it. On the other hand, if you have a property called animationDuration I'll be safe because you will just change the getter for that property and everything will work. (You can create dynamic properties where you provide the getter/setter methods).

Etabubu wrote:- Both class and instance methods need to follow Apple guidelines in regard to naming conventions

Please be more specific with your proposed changes and I will consider them.

Sure. This is a good doc to start with: https://developer.apple.com/library/mac ... lines.html
In this specific case:

1) All the "create:" methods should be called "skeletonWithFile: ...". The word "create" is discouraged by Apple because creates confusion about the ownership. Factory methods are usually named "<ObjectReturned>With<Param>".
So:

+ (id)skeletonWithFile:(NSString *) atlas:(Atlas *)atlas;
+ (id)skeletonWithFile:(NSString *) atlas:(Atlas *)atlas scale:(float)scale;
+ (id)skeletonWithFile:(NSString *) atlasFile:(NSString *)atlasFile;
+ (id)skeletonWithFile:(NSString *) atlasFile:(NSString *)atlasFile scale:(float)scale;
+ (id)skeletonWithData:(SkeletonData *)skeletonData;
+ (id)skeletonWithData:(SkeletonData *)skeletonData stateData:(AnimationStateData*)stateData;

2) Init methods should return "id" and should follow the same naming conventions:

- (id)initWithData:(SkeletonData *)skeletonData;
- (id)initWithData:(SkeletonData *)skeletonData stateData:(AnimationStateData*)stateData;

3) Only property setters should start with "set", so... suggestions on how to rename all your "set" methods:
"setMix..."

- (void)mixAnimation:(NSString *)firstAnimation withAnimation:(NSString *)secondAnimation duration:(float)duration;

"setAnimation..."

- (void)startAnimation:(NSString *)animationName loop:(BOOL)loop;

And so on...

4) In getters, never use the prefix "get". So...
"getAttachmentForSlotName..."

- (Attachment *)attachmentForSlotName:(NSString*)slotName attachmentName:(NSString*)attachmentName;

AnimationState does not have an explicit method to stop animating. setAnimation(null) can be used. I will add a clearAnimation method though, as it is generally less nice to overload a method parameter to use null as a special case.

Actually, if I use the ObjC setAnimation: method and I pass NULL, it crashes.
Beside that, as I said above, this shouldn't be called "setAnimation" because it's not a setter. Just to be clear, I'm ONLY talking about ObjectiveC conventions and I'm not suggesting to rename spine-c functions!
A "startAnimation/stopAnimation" or "startAnimation/clearAnimation" pair of methods would be much appreciated. That's also what you usually find in ObjC APIs. You'll never find a method that expects "nil" to perform a different or opposite action. But I'm well aware that it's common practice in C/C++.

About the delegation pattern: keep in mind that you can implement it in the ObjC runtime without having to touch the C code. To be clear: I don't want you to implement a notification system in spine-c, but only in the ObjC wrapper. It's possible, I've done it yesterday, without touching your class. But it would be much cleaner if it was implemented by you in CCSkeleton.

Final words: the current state of the CCSkeleton class makes it relatively useless. Yes, it helps a lot to render the animations in Cocos2D (and I know this is its main purpose), but it lacks a clean ObjC interface to access all the internal data and to perform non-rendering related tasks (such as playing with bones and slots).
IMHO, if you just wanted to create a Cocos2D renderer for Spine, you could remove all the "find...", "set..." and "get..." methods because people can just use the spine-c API to do all that stuff.
It looks like you're in the middle between a simple Cocos2D renderer and a full blown ObjC wrapper. (I'd like to have the latter 🙂 ).

I'll stop here, otherwise this post becomes way too long and then it looks like I want to teach you stuff. 🙂
These are just suggestions from one of your customers. You can then do whatever you want with them.
Hopefully this post won't sound like "criticism nonsense" and will give you some tip on how to improve the ObjC API.

No hard feelings! Spine is a fantastic tool and it was really needed in the 2D games scene. That's why we bought it 🙂

Thx,
Marco

Nate

Etabubu wrote
Criticism nonsense? I actually suggested you how to improve the API with a list of points and reasons. That's called constructive criticism...
When I said "poorly written" I wasn't talking about the spine-c API or its internal design, but I was referring to the ObjC interface, which is, in fact, "poorly written".

Name calling is never constructive. 🙂 The rest of your post which dealt with specific issues was constructive. Anyway, we can move on.

but I just read another post where someone was asking a similar thing and you decided to implement the "isComplete" flag to the C structure

I added isComplete because you asked for it in your last post. I was just pointing it out to someone else.

What you would end up doing is rewriting the entire runtime, duplicating all the spine-c data structures and logic in ObjC.

No, you don't need to go that way of course. I'm not asking that! 🙂
But for example, if I want to retrieve the current animation time, or duration, or name... or any other field in that structure (and other structures you're using), I should be able to access them through properties. Even if they're just readonly properties.

spine-c is a C API. They are structs, there are no properties. The only way to provide property access is to wrap the structs in an ObjC API. As I explained, this will result in rewriting the entire runtime in ObjC. So this is in fact what you are asking.

Yes, if I change things it can break your code. This is just life. It isn't possible to use properties to access spine-c data.

1) All the "create:" methods should be called "skeletonWithFile: ...". The word "create" is discouraged by Apple because creates confusion about the ownership.[snip]
2) Init methods should return "id" and should follow the same naming conventions:

I vaguely remember I used "create" because I saw it in cocos2d somewhere... ah, turns out it was a cocos2d-x convention. I see the cocos2d methods use the conventions you described, so I'll make the changes. I did the id change yesterday.

3) Only property setters should start with "set", [snip]
4) In getters, never use the prefix "get".

These methods are for convenience and meant to parallel the spine-c API. I think it is more clear what is going on if they have the same name as the spine-c method they call.

About the delegation pattern: keep in mind that you can implement it in the ObjC runtime without having to touch the C code.

Yes, the issue is I have a feeling that functionality would not be useful in all cases and I'm not sure how common using it would be. I'm not against adding it for convenience, but would like to see more how the API is actually used in practice.

IMHO, if you just wanted to create a Cocos2D renderer for Spine, you could remove all the "find...", "set..." and "get..." methods because people can just use the spine-c API to do all that stuff.
It looks like you're in the middle between a simple Cocos2D renderer and a full blown ObjC wrapper. (I'd like to have the latter 🙂 ).

I had this same thought, and I agree. I've removed the spine-c methods to make it clear that the normal way of doing things is to use the C API. If there was a reasonable way to do an ObjC wrapper, I would do it. Give it a try yourself, you'll find that the relationships between objects leads you to rewriting the whole runtime. 🙂

As I said, I'm not offended and there are no hard feelings. I enjoy discussing these things. While I know you haven't quite gotten everything you would like, I do think the code has been improved from this discussion and you are more than welcome to continue sharing your ideas. 🙂

infrid

I appreciate how Nate is having to keep the wrapper lean to save from rewriting the underlying c api from spine, but I don't buy this "have to rewrite the entire runtime" business.. couldn't we at least get some properties and helper methods that do a bunch of boilerplate c code which we have to do by hand (incidentally without rewriting the entire c runtime).

The objective c runtime is pretty pants in comparison to say the java runtime, lots of features are missing.. "just go look at the c runtime" you guys say.. oh yes.. because it's so well documented isn't it.. :/

I personally think you guys should invest a bit more time adding some common api features that are missing, or produce exhaustive api docs.. I certainly think the former would be quicker for you guys and better for everyone than the latter.

Etabubu

I'm glad we're finding an understanding.

I have to correct you on this though:

spine-c is a C API. They are structs, there are no properties. The only way to provide property access is to wrap the structs in an ObjC API. As I explained, this will result in rewriting the entire runtime in ObjC. So this is in fact what you are asking.

Yes, if I change things it can break your code. This is just life. It isn't possible to use properties to access spine-c data.

You don't need to rewrite ANYTHING to add properties to the ObjC API.
Look, in the header file:

@property (nonatomic, readonly) NSString *animationName;
@property (nonatomic, readonly) float animationDuration;
@property (nonatomic, readwrite) BOOL debugBones;

And in the implementation file:

@dynamic animationName;
@dynamic animationDuration;
@synthesize debugBones = debugBones;

- (NSString *)animationName
{
    if (state->animation == NULL)
    {
        return nil;
    }
    

    return [NSString stringWithCString:state->animation->name encoding:NSASCIIStringEncoding];
}

- (float)animationDuration
{
    if (state->animation == NULL)
    {
        return 0;
    }
    

    return state->animation->duration;
}

Supereasy to add a property for almost all the data you have in the C structs!
Breaking the code of your customers is not "just life", since it can be avoided through a cleaner interface.
Backward compatibility is actually one of the most important features of an API. Of course, at the very beginning of the development it can't really be avoided. But after that, it becomes really frustrating.

Last thing I forgot about ObjC conventions.
All your instance variables (doesn't matter if protected/public/private) should be prefixed with an underscore "_". Apple is becoming stricter and stricter about it. You can check this in Cocos2D. All classes conform to this.

Ok, thanks for your time, Nate.
We'll see where this goes.
I'll have fun working with Spine anyway! 🙂

Marco

infrid

thanks for adding this Marco.

Sorry nate but you look plain lazy and ignorant here [edit - and arrogant and like you don't take your customer's needs seriously] - I'm going to shout this out on the cocos forums.

As a paying customer who also paid for the coding video workshop, which without reading through reams and reams of c code, I can not use; I am not impressed. I bet I am not the only one. We shall see.

In the end I think cocos2d had the most votes from every runtime, by a clear head - and we get the shittest api.. yep.. that makes sense doesn't it.

Nate

infrid, name calling and cussing is not acceptable here. You are being rude, so I won't respond to you. If you continue to be rude, you will lose your posting privileges here.

Nate

Etabubu wrote
Supereasy to add a property for almost all the data you have in the C structs!

Your example only exposes a few simple properties. The spine-c object model has a lot more data that is more complex. SkeletonData has arrays of bones, slots, skins, and animations. Slots reference a bone and attachment, skins reference a number of attachments, animations have an array of timelines, etc. If you expose the arrays as NSArray or its variants, then you will have an ObjC array and a C array which have to be kept in sync. If you expose the array of structs as is, then you are exposing structs and not ObjC classes, so you are back to struct fields which have no ObjC properties. Again, the C API can't be wrapped nicely in ObjC without rewriting the whole runtime in ObjC. In my opinion this does not have enough advantages to make it worthwhile.

All your instance variables (doesn't matter if protected/public/private) should be prefixed with an underscore "_".

I agree, especially for private fields exposed only through properties. For public fields I prefer not using the underscore.

Etabubu

I agree, especially for private fields exposed only through properties. For public fields I prefer not using the underscore.

I don't think you realize what it means when Apple recommends to follow certain conventions. Obviously you're not familiar with the iOS/OS X development environment.

Anyway, it's pretty clear by now that you won't change your mind, no matter how many different explanations I'd provide, so I'll stop bothering you.

Good day,
Marco

bignsyd

Considering exposing instance variables as public is not a recommended practice I'd be interested in seeing a link to the documentation. I've read through most of apple conventions and standards but don't remember reading any recommendations using underscores for publicly accessed instance variables.

Nate

I think having or not having underscores for field names is a very minor issue. The same is true for properties or direct field access. Naming and coding conventions are important, but it is much more important that the API allows users to be productive. Sure, it would be nice to have an ObjC runtime API for cocos2d, but the C API works perfectly fine there. No one seems to be able to explain why an ObjC runtime would be so much better, so I don't see any reason to do the extra work. It isn't about being lazy, it's about the advantages of having less code: less maintenance, less effort to develop new features, less code and more eyes on the same code means more testing and fewer bugs, etc.