MrStahlfelge/gdx-gamesvcs

GPGS Desktop implementation

mgsx-dev opened this issue · 11 comments

As discussed here : http://www.badlogicgames.com/forum/viewtopic.php?f=17&t=26608&p=104851&hilit=gamesvcs#p104851 I'm working on GPGS Desktop implementation and I have something working pretty well. I'll PR you when all will be clean.

You can have a look at my dev branch https://github.com/mgsx-dev/gdx-gamesvcs/tree/desktop-gpgs
there is a test GUI (GpgsClientTest) if you want.

If you don't mind, I will use this issue to ask some questions about how to implements it correctly and make some suggestions about current API and other implementations.

Threading

I understand your concept of "fire and forget" : user code call a methods and get result in listener but ...

Questions :

  • Is IGameServiceClient implementations have to be thread safe ? my opinion is it don't have to be.
  • Is IGameServiceClient implementations have to be called in GLThread ? my opinion is it don't have to be.
  • Is IGameServiceClient implementations have to be non blocking ? that is process things in background. My opinion is it shouldn't (see below).

While underlying library may do things in background (activity intent in android...), I think GameServiceClient should never spawn threads itself, it is the user code responsibility to do it, to not doing it, to parallelize some tasks or chaining multiple calls in a single background thread and synchronize things if needed.

It is not really clear what implementation is supposed to do. I looked your android-gpgs implementation and most of methods are blocking except the load/save game state. I don't understand why mixing up blocking and non blocking parts.

I need more information on this before continue my implementation.

Packaging

I saw all implementation in the same package (de.golfgl.gdxgamesvcs) which may conflicts if you use multiple implementations in the same app (I may want to use both GPGS android and GPGS REST in my application).

I suggest to use the convention : de.golfgl.gdxgamesvcs.[service].[platform]

Versionning

I saw that android-gpgs-0.0.1 depends on android-0.0.1-SNAPSHOT ... have to wait the next release to be solved :-/

I see also that next release is 0.1.1 and not 0.1.0 ... maybe a mistake ?

and another quick question : what VCS stand for ? :-)

Features

In my original implementation, I have features that i can't implements with your API like :

  • listing all achievements
  • fetching leaderboards
  • list game states
  • delete game state

I understand that android implementation delegates a lot to google play activity but it won't be the case for other implementations. Maybe API is too tight to GPGS concepts and workflow ... It could be a little more abstract in my opinion, what do you think ?

I hope you dont't take all my remarks too badly because even if there is some things to fix up, current API is globally usable.

Hi, thank you, I am appreciating your efforts. 👍

I'm in vacations from tomorrow for 10 days, so I may not get back to you in this time.

Please also use my test app for testing your implementation. It does not cover all specifications, but the implementation should work with it.

Threading

Is IGameServiceClient implementations have to be thread safe ? my opinion is it don't have to be.

No, calls should be synchronized by the caller.

Is IGameServiceClient implementations have to be called in GLThread ? my opinion is it don't have to be.

This is my opinion, too, because I don't see a case where a service client need to act with the UI.
But: in all my applications all methods are indeed called on the UI thread. That must work to keep the things simple for the calling application. Have in mind that this lib should not only provide a service-independant interface and different implementations, but should also provide access to the services for beginners.

So this is why the answer to this question:

Is IGameServiceClient implementations have to be non blocking ?

Is yes, they should be non-blocking so that the UI thread won't be blocked.

I looked your android-gpgs implementation and most of methods are blocking

Which one did you find that is blocking? They are all non-blocking, otherwise that is a mistake.
The methods of all other client implementations are non-blocking, too. Exceptions are the sync and get Methods.

Packages and versions

I saw all implementation in the same package (de.golfgl.gdxgamesvcs) which may conflicts if you use multiple implementations in the same app (I may want to use both GPGS android and GPGS REST in my application).
I suggest to use the convention : de.golfgl.gdxgamesvcs.[service].[platform]

Hm, I wonder if this problem can really occur. If you want to use both Gpgs Clients, the one for Android is just added to the Android package while the REST API client is added to desktop. So there should be no conflict. I don't see a case where different implementations for the same service need to be packaged to one platform.

I also realized the wrong dependency on my 0.0.1 release. :-( Shame on me. This will not happen the next time as I know where my mistake was, but this was my first release on a public repository.

Going to 0.1.1 was no mistake, is it a problem?

svcs is short for SerViCeS. ;-)

Features

Your feature requests are absolutely valid. I know that the interface is very Android specific at the moment. It is not sufficient for desktop and HTML clients without a service-provided interface.

But it won't be easy to define a interface on this part as it was for submitting data.
I took a look an many game services the past weeks and when it comes to leaderboards especially, the data structures vary a lot. Most, but not all provide daily/weekly/alltime leaderboard entries, some services can fetch the neighbouring board entries, only some can return if you are within the top 10% of players... so I fear that fetching leaderboards will lead to different interfaces. And that is my suggestion for you: For your required features, extend IGameServiceClient (indeed I did that for my games, too) and we will discuss your interface.

I hope you dont't take all my remarks too badly

No problem, I am not offended at all. Such discussions will lead to a better documentation and a better library. If you feel that some of the points above are not stated or not stated clearly enough on the interface's JavaDocs, feel free to extend them!

Thanks for your answers, we will discuss all these topics when you're back, have a good time ;-)

Threading

OK, I understand the philosophy of your API and it sounds good to me but it should be stated more precisely in the IGameServiceClient javadoc (I'd prefer that you make the changes) :

  • explain implementation doesn't have to be thread safe and recommend user code to call it from GLThread (typical usage).
  • explain methods of this interface are non-blocking : you could move the "fire and forget" concept to the interface javadoc instead of repeat/omit on each methods javadoc.

Which one did you find that is blocking?
Sorry, I checked android javadoc and indeed you're calling non blocking methods, I saw that "fire and forget" is from this javadoc ;-)

Packages and versions

For the "all in the same package" topic, I'm OK with that, I thought using desktop implementation in android for debug/dev purposes but I realized it is not very useful since debugging can be done in desktop version.

Going to 0.1.1 was no mistake, is it a problem?
There wasn't 0.1.0 version, it's not a big deal to jump version but it could be confusing like "I have trouble with 0.1.1, I'll try to switch to 0.1.0 but it doesn't exists ..."

Features

I think you're right and for specific use cases maybe subclassing is the way to go.

Here is for instance how I plan to implement the "showAchievements" :

  • GPGS Desktop has no native interface and showAchievements method throw error by default.
  • Client code typically subclass this implementation and override showAchievements.
  • Client code can then call Implementation specific methods like "fetchAchievements" in order to build the GUI.
  • Achievements GUI is typically in the user core package and shared with other platforms.

I will demonstrate this in my test application.

For the saved games features (list, delete), I think we could find something portable across implementations. As you're suggesting, I will extends the interface and you'll tell me.

Demo and Tests

I realized your test app was in another repository. I suggest to move it in the gdx-gamesvcs repository like LibGDX does.

I tried your demo with my implementation and it's work well but doesn't cover all my test scenarios so I think I will keep mine anyway. Since these tests are implementations dependent I think it doesn't worth to try to unify them in some way.

I will rework the interface documentation as you suggested when I'm back. And yes, "fire and forget" is mentioned by the Google docs. :-D

For your sugestions according the new features, yes, that's a way to go. If possible, fetch* operations should be also callable from the core interface as you say.

For example, the following approach for achievements should fit to all game services I know. Saved Games should be also no problem.

  • Achievement class with members ID, label, % earned, boolean incremental
  • IFetchAchievementsResponse interface with method onAchievementsResponse(boolean success, List Achievements)
  • New IGameServiceClient method fetchAchievements(IFetchAchievementsResponse response) throws NotSupportedException
  • New IGameServiceClient method canFetchAchievements()

If the demo is working with your implementation, that is very good!
I kept it in another repository following the guide by @TomGrill http://tomgrillgames.com/how-to-write-your-own-libgdx-extension/.

I think I have something pretty good right now, I let you check my branch. Since I didn't want to modify other implementations, I extended current interface with the new features, we could merge them progressively when they become more mature.

I took some liberty about your suggestions and they are opened for discussion for sure. Among them :

  • all callback interfaces are named *Callback and result passed to these callbacks may be null which states a failure. This avoid to have redundant boolean status with a null result.
  • a proposal about exceptions handling : since user code check if a feature is supported or not, why declare checked exception. As you can see in my implementation I just throw an unchecked exception for debugging purpose. I think it simplify client code.
  • instead of adding a lot of provides*, I proposed a unique isFeatureSupported method (like LibGDX do with Peripheral), this simplify things notably in case of new feature, we don't have to change other implementations.
  • I introduced a "showGameStates" method. While no implementations handle this at this time it provides an homogeneous interface though.

Note : Leaderboard interface I implemented is quite complex, I think we have to discuss about it.

I also implemented interface methods in a "beginner fashion way", that is all calls are non blocking (like your implementations) but callbacks (and listener) are automatically executed in GLThread. I think this is simpler for users to not bother themselves with concurrency. Workflow is simpler :

  • Call a non blocking method from a Button
  • Display the GUI in the callback.

For advanced usages, user code can directly call all the *Sync methods and implements their own threading strategy.

Obviously, my branch still a work in progress, I think you shouldn't merge anything right now, I could provide individual PR if you wish, the branch history is a little messy and I plan to rebase/squash it when it will be more stable.

I looked over your new branch and will check further and try it out when there is a version you approved for others to test. Individual PR is not needed, I can use your repository as origin for my local repo.

I just have two remarks so far.

For the leaderboards, for most other game services an integer parameter for the maximum number of leader board entries to return is needed, some even support a range param. On the other hand, filtering by friends or just returning the entries around the player is not supported by all services. One approach could be to ignore these parameters in this case and to have isFeatureSupported enumerators to know whether the parameters can be used, but perhaps we should overload the method so that it can be used with less parameters? Or is a parameter object the better choice? I am not sure.

Returning the responses on the UI thread: I see that this is easy to use for beginners. But calling Gdx.app.postRunnable is not difficult, you just have to know the method's name. The UI thread is suspended on Android devices if the app is in background and not needed for processing the responses in all cases.

Everything else, well done - looks promising and I am looking forward to try it. 👍

You can already test it from my branch.

I didn't found a way to test leaderboards with multiple accounts. I have 2 accounts but I can't see both players in the leaderboard. Could you test it for me with one of your app credentials ?
The main thing to test is if players order in the list is correct.

Leaderboard

Leaderboard API is not easy to define as we discussed. To avoid a too complex API, we could implement the most common use case : show the top 100 players and the current player. If coders want something more accurate (social only, pagination, ...), they could always override the service and implement their own logic.
We could forget about it right now and define a better API later if it worth...

UIThread

for the UIThread topic, my opinion is that it's not a matter of how difficult is to call the postRunnable method but a matter of design and user code safety and "pollution". Question is what is the use case of processing result out of UIThread ? IMO there is no such use case. Any remote API could be defined in 2 ways :

  • ASYNC (safe mode) : user code doesn't have to deal with threading, API calls are non blocking and callbacks are always in UIThread.
  • SYNC (expert mode) : user code implements its own threading logic, API calls are blocking and result is return directly from calls.

I implemented both approaches in my desktop implementation which let user code to call *Sync method if he want.

API changes

Last question : what do you think about the IGameServiceClientEx interface ? excepting leaderboard, do you think we could merge it to the IGameServiceClient ? that is :

  • replacing all the "is***Supported" by isFeatureSupported
  • adding all the new methods

If so, I prefer you do it, I don't want to change code for other implementations since I can't easily test them with appropriate accounts.

Ok, I'll test it. :-)

I will adopt your interface enhancements in issue #4 and check against the other game services.

But with the UI thread, I am not convinced. But I think I've found a very libGDX-ish way we both are satisfied. But we should also seperate this topic from this issue.

I adopted all possible core changes.

I hat to make the following changes to your interfaces:

Achievement, leaderboard and user pictures: I ripped them off. Feel free to extend the classes for your needs, but generally, I would suggest not to load the leaderboard and achievement pictures from the game service but have them in your game. Pictures for players are also not supported by every game service, handling is very different and in the most cases users cannot even set an own picture.

return boolean on the client methods: I changed from void to boolean like the other methods are. Of course they cannot return success because they are asynchronous, but they can return if even an attempt to query was made.

Leaderboard and fetchLeaderboard have the most changes to reflect what can be done with the most game services. I removed possibiltiy to get leader board description and picture for the same reasons as the user pictures. I suggest not to use descriptions and names fetched by the game service but use own within your game (also regarding localization), use the description from the game service just for their own user interface.

I provided some RenderThread response listener wrapper examples. You can now get what you want bycalling
gsClient.setListener(new GameServiceRenderThreadListener(yourListener))

I hope this fits your needs. Next I will test your implementation, I am very curious.
The reason why you did not see your leader board entries is probably because of your privacy settings. Many users have visibility deactivated, unfortunately.

I also changed the members from public to protected. There are no set methods, it should not be possible for the clients to set the values. But perhaps you need to use an own subclass to set them. I am not sure if that is a good approach, but public set methods also didn't seem to be good.

I merged the current 0.1.2-version to your branch, that worked without conflicts. I checked against my test app. Indeed I get a browser window and can succeed. This is really cool. My with the desktop client submitted leaderboard entry is immediately shown on Android's own interface and achievement unlocking some time later.

I will report found issues here.

  • There are some JavaDoc errors when performing uploadArcvhives gradle task
  • I get some warnings when starting the application:
    unable to change permissions for everybody: ...\gdx-gamesvcs-desktop-gpgs
    unable to change permissions for owner: ...\gdx-gamesvcs-desktop-gpgs
  • logOff does not report gsDisconnected to listener
  • after logging off, calls to submit methods keep trying to submit but throw a NPE
  • after logging off, connecting next time succeeds. On Android, after logging off a new authentication is needed and silent login does not work any more. This is not important for me as not every game service handle it this way, but I wanted to let you know that users might expect same behaviour on different platforms.

I did not test the fetching functions, I did not get your test client to run. I will extend my test app for fetching functions for version 0.2x and can then test it when you adopted to it. Thanks for your work so far!

Thank you very much for the implementation. I did some minor changes to the classes, you can see the commits.
I changed the core Achievement and LeaderboardEntry to interface and added some missing leaderboard information. Perhaps an isGuestEntry() method is needed on leaderboard entry.
I think most important for you is that I added a Gdx.app.error - we have different approaches regarding logging, but this is to keep it consistent with the other implementations. :-) Also, I removed your test client but was able to test your implementation with my client perfectly.
Good work, thank you.

I added a Wiki section for your implementation, please review it.

Can you add submit events functionality? Should we open an extra issue for it?

Do you want to announce in libGDX forum?