Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Title API for Minecraft 1.8. #156

Merged
merged 1 commit into from
Nov 4, 2014
Merged

Add Title API for Minecraft 1.8. #156

merged 1 commit into from
Nov 4, 2014

Conversation

stephan-gh
Copy link
Contributor

Adds an API for the Titles added in Minecraft 1.8. There is already a pull request in #106, but this one is slightly different:

  • Fluent API, multiple calls can be chained.
  • Added missing ways to clear or reset the titles.
  • Improved and more detailed Javadocs.

Example Usage

player.sendTitle(game.createTitle()
    .title("Sponge")
    .subTitle("http://www.spongepowered.org/")
    .stay(2400) // Stay for 1 minute
);

// Reset the previously sent title
player.resetTitle();

Edit by handler:

YouTrack

@simon816
Copy link
Contributor

Hmm wonder whether this could use text formatting from #15 or #47, it would allow a unified way to color text etc (If one gets pulled)

@stephan-gh
Copy link
Contributor Author

@simon816 Yes, this needs to be added once one of them is pulled.
See https://github.com/SpongePowered/SpongeAPI/pull/156/files#diff-60179b7748b625bbdfceb511d6adf780R47

@Amperial
Copy link

If you added a Player#resetTitle() you could remove Title#clear() and your example would become:

player.sendTitle(game.createTitle()
    .title("Sponge")
    .subTitle("http://www.spongepowered.org/")
    .stay(2400) // Stay for 1 minute
);

// Reset the previously sent title
player.resetTitle();

Having to reset a newly created title is kinda ridiculous, as well as having to type a giant line (game.createTitle().reset().send(player);) just to clear a player's title if you don't have a reference to a title already.

@nikosgram
Copy link

@AgentOak
Copy link

I think ampayne2 is right, the way to reset a title is odd. Title should be a simple class just containing the few variables a Title has, doing nothing else. The magic happens in Player#sendTitle(). I'd even remove Title#send() to have a simpler API. No need to have the same method twice, it'll just confuse people.

@stephan-gh
Copy link
Contributor Author

@ampayne2 Even though we could add this method additionally, the reset and clear methods should both stay in the Title interface because they can also be used in a more complex title configuration like in my first example above. If we would remove reset and clear then you would need to have an additional line just to reset the title first.

player.sendTitle(game.createTitle()
    .reset() // Reset the previous title first
    .title("Sponge")
    .subTitle("http://www.spongepowered.org/")
    .stay(2400) // Stay for 1 minute
);

Instead of the simple reset() here in the second line you would need to add a new player.resetTitle() for every player you're sending the title to. The title configurations are designed so they can be used multiple times (also to save performance), so you could just have this when your plugin is loading:

private Title titleConfig = game.createTitle().[...];

And then send it to different players again, e.g. when a command is executed. Adding a resetTitle() method would only reset the title once for a single player. We should not add that much duplicate API that's why I don't think we should add this. Plugin authors can always create their own methods for easier usage.

@Marco01809 I agree, but Title#send() is easier to use if you want to send it to multiple players, like this. Player#sendTitle() has been added because if you have a player you would look for a way to send titles there first. The methods actually have not that much difference so I can remove one of them if some others agree with this.

@nikosgram13 I'm the author of BungeeCord's Title API, I have just ported it to Sponge. :)

@Amperial
Copy link

@Minecrell Is it not possible for Player#sendTitle() and Title#send() to automatically reset the player's current title if existing?

import org.spongepowered.api.entity.Player;

/**
* <p>Represents a configuration of a title. It consists of a main title and a

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<p> tags shouldn't be on the first paragraph

@turt2live
Copy link

From the looks of it, this PR seems to imply that something like this is possible. Please correct me if I'm wrong.

private Map<UUID, Title> titles = new HashMap<UUID, Title>();

public void doSomething(UUID uuid){
    titles.put(uuid, game.createTitle()); // Obviously this would have changes and compile
}

public void doSomethingElse(){
    for(Map.Entry<UUID, Title> entry : titles.entrySet()){
        entry.getValue().send(player); // player is found via uuid, somehow. 
    }
}

@turt2live turt2live self-assigned this Sep 14, 2014
@turt2live
Copy link

@Minecrell Is there an implementation PR for this? If so, please link it!

@stephan-gh
Copy link
Contributor Author

@turt2live Sponge is not yet using 1.8 so I don't have an implementation for Sponge right now, but I have a similar (as of right now, only older Javadocs) API implemented on BungeeCord using packets: https://github.com/SpigotMC/BungeeCord/blob/master/proxy/src/main/java/net/md_5/bungee/BungeeTitle.java
I'm not yet completely sure in what extend this can be implemented similar on Sponge - need to look into the source once it is available. However - I did plan to implement it similar.

Saving Title instances would be actually the recommend way to use them, because it can (depending on the implementation) save performance when sending them multiple times. That's why your example should work very well, and perform better than with most other API designs.

On BungeeCord, performance is saved by creating the packets only once on creation. The same applies to the serialization of title and sub titles. For Sponge pre-serializing the title and sub title will be probably not possible, because the packet classes use the chat components directly and will be serialized when the packet is encoded.

I don't know if the Minecraft server has its own utility classes for sending titles, in that case this API design might not have so many advantages over others.

@ampayne2 This is not possible. Simple title packets can be send by anything, and controling all command blocks as well as custom packet sends by plugins would be nearly impossible. I've got some ideas how the API could be improved to simply this, will try to implement this later.

@turt2live
Copy link

@Minecrell This PR doesn't really help if there is no way for it to be implemented at this time. This PR will be left on hold until possible.

@stephan-gh
Copy link
Contributor Author

@turt2live This pull request is waiting for multiple things right now. An API for text formattings is required as well. I will implement all missing things once they're available.

@Amperial
Copy link

@Minecrell I looked at your BungeeCord implementation and it makes perfect sense now. Just looking at the API makes it seem like reset() is only resetting the values of the Title instance when it actually creates a Title packet with Action.RESET which gets sent before the rest of the packets. It would be possible for a reset packet to always be sent without reset() but then it wouldn't be possible to NOT send it. The only improvement I can think of then would be adding a player.resetTitle() that does game.createTitle().reset().send(player);

@stephan-gh
Copy link
Contributor Author

@ampayne2 That's what I'm currently thinking about. I had 2 different ideas how to implement this right now:

Title manager / factory Game / Player Description
create() game.createTitle() Creates a new title and resets the current one first.
update() game.updateTitle() Like above but without resetting - can be useful if you want to update a countdown for example.
clear(Player) player.clearTitle() Clears the title from the player's screen.
reset(Player) player.resetTitle() Resets the title from the player's screen.
game.getTitleManager() Get the title manager / factory.

In this case I would probably agree and go for the second version because it is easier to figure out how to use, not yet completely sure, however.

@turt2live By the way, I didn't open this to get it merged as fast as possible - this is more designed as a discussion for a possible API solution. I'm aware that it is not yet useful as of right now. :)

@stephan-gh
Copy link
Contributor Author

@ampayne2 I have changed this now and added player.clearTitle() and player.resetTitle(). :)

@stephan-gh
Copy link
Contributor Author

I don't think the failed Travis build is caused by my pull request:

/home/travis/build/SpongePowered/SpongeAPI/src/main/java/org/spongepowered/api/math/EulerDirection.java:56: method does not override or implement a method from a supertype
    @Override
    ^
1 error
:compileJava FAILED

@rbrick
Copy link

rbrick commented Sep 18, 2014

^ Someone forgot to extend Comparable

@stephan-gh
Copy link
Contributor Author

@rbrick Yeah, looks like it is missing in d4a088e

@turt2live
Copy link

@Minecrell Your PR is currently not mergable. Please resolve the issue to have it reviewed further, thanks!

@stephan-gh
Copy link
Contributor Author

@turt2live Fixed! :)

*
* @return A new clean {@link Title} configuration.
*/
Title createTitle();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So...there's only one Title per game?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be multiple Title configuration instances at one time (all generated by Game), but only one can be displayed by the client at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Can I not have a different Title for each Player?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're asking. You can have as many Title configurations as you want, but one client (= one player) can only display one title at the same time. The advantage of this API is that you can also use the same Title configuration for multiple players and multiple times, but of course this is not required. You could also create a Title configuration just for one player.

For example, both of these usages are perfectly fine:

Title title = game.createTitle().title("Sponge").subTitle("http://www.spongepowered.org/");
for (Player player : game.getOnlinePlayers()) {
    player.sendTitle(title);
}
for (Player player : game.getOnlinePlayers()) {
    game.createTitle().title("Sponge").subTitle("Playing as " + player.getName()).send(player);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need updateTitle and createTitle then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods are used to obtain an instance of a new Title configuration. The difference between createTitle and updateTitle is just that createTitle will clear the current Title settings on the client before sending the Title configuration.

@stephan-gh
Copy link
Contributor Author

@turt2live You were asking for an implementation PR of this. This is still not completely possible, but here is how the title configuration class would look like in the mc1.8 branch of Sponge as of right now: https://gist.github.com/Minecrell/80f6ff8b28e5f669114c
This is not yet optimized (the Minecraft packet classes have no setter..) that's why we need to recreate the packets every time for now.

@Lunaphied
Copy link

Can you make a working version on a fork branch?

@Lunaphied Lunaphied self-assigned this Nov 3, 2014
@stephan-gh
Copy link
Contributor Author

@modwizcode There is no real way to implement this as of right now, because it requires Forge 1.8. I can already create the implementation class for Title (see above) using the mc1.8 branch, but it won't work until the Player class is implemented in Sponge.

@Lunaphied
Copy link

I have an implementation of Player for sponge on the master trunk. It's a wrapper, but that's the only real way to implement it before @Mumfrey fixes mixins.

@stephan-gh
Copy link
Contributor Author

I can't implement from in the master branch because it requires packets only available in Minecraft 1.8. Also, I can't seem to be able to find the Player wrapper in the Sponge repo, there is just a BlockWrapper in there. Could you link to the file here?

@Lunaphied
Copy link

Oooooh I might've put that in for events. Checkout this:https://github.com/modwizcode/Sponge/commit/e216a660d77139949aa432722c13b5d831cd39e4. That branch should be good for the implementation I want to see. It's what I'm working on actively and I just want to fuss with an implementation.

@stephan-gh
Copy link
Contributor Author

Is this state actually able to run so I can test if it is working? I'm not quite sure about the current Forge/FML state of 1.8, if the server doesn't start I will be only able to create another draft. By the way, if you are searching for a complete working implementation of a similar API to this one you could also take a look at BungeeCord, an older version of the API (improved it a little bit) is implemented there: SpigotMC/BungeeCord@4e353e9

@Lunaphied
Copy link

Oops I'm sorry I wasn't thinking clearly, my mistake. I will try and get something figured out. Just copy that player wrapper into the mc1.8 branch and use that. That's the one I will use anyway.

@stephan-gh
Copy link
Contributor Author

@modwizcode After an hour of work I got it working now. Had to figure out how Sponge is actually working first. I did the following things now on my forked repo:

It is working fine, if you join the Sponge server then the title it will be displayed and stay for the specified amount of time.

@Lunaphied
Copy link

Thanks! That's great!

Zidane pushed a commit that referenced this pull request Nov 4, 2014
Add Title API for Minecraft 1.8.
@Zidane Zidane merged commit d62ee25 into SpongePowered:master Nov 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants