Advancements API #1554

Cybermaxke · 2017-05-10T19:12:26Z

This PR adds the advancements API, with additions extra to the features provided in vanilla Minecraft. Implementations will follow when 1.12 is released.

The AdvancementTree represents a advancement Tab, it can be build with a root advancement. Advancements that are attached to the root advancement will be present in the tree. The layout of the tree can also be customized, which means that you can move the advancements around in the tab though a AdvancementTreeEvent.GenerateLayout event.

AdvancementConditions are the conditions that should be met before an advancement is unlocked. I provided AND and OR operations that can be applied. There is not limit in the complexity like in the json files.

There is also Triggers that can be applied to the AdvancementCondition, the details of vanilla triggers are not exposed in the API (the configuration). However you can create your own Triggers which will be parsed in the advancement json files.
http://minecraft.gamepedia.com/Advancements#Triggers

Additionally there is a ScoreAdvancementCriterion which requires a specific goal value to be achieved before it gets done, this is also supported by triggers. The trigger will need to trigger the criteria multiple times until the goal is reached, this is also supported in the json files by adding a trigger_times property to the trigger.

Extras:

Generic Events - Used for the registrations of advancements and trees.
ScoreAdvancementCriterion - Allows you to provide a goal value and when the score reaches its goal the advancement will be unlocked. Also supported in json files.
Complex AND and OR condition operations
Custom triggers (configurable) and supported in json
Customize the advancement tree layout.

stephan-gh · 2017-05-10T19:16:35Z

src/main/java/org/spongepowered/api/advancement/criteria/AdvancementCriterion.java

+     * @param criteria The other criteria
+     * @return The and operation
+     */
+    static AdvancementCriterion buildAnd(AdvancementCriterion... criteria) {


I don't think the build prefix is needed here

Otherwise will it conflict with the non-static and method.

You can make it static AdvancementCriterion and(AdvancementCriterion adv1, AdvancementCriterion... therest)

kashike · 2017-05-10T19:23:16Z

This should target the 1.12 branch.

kashike · 2017-05-10T19:42:19Z

src/main/java/org/spongepowered/api/advancement/Styled.java

+ * Some that has a style within a advancement tree. Either an
+ * {@link AdvancementTree} or an {@link Advancement}.
+ */
+public interface Styled extends CatalogType, TextRepresentable {


This name should be more specific.

Cybermaxke · 2017-05-10T20:46:02Z

@kashike I don't think that this PR will be finished before the 1.12 branch is merged. Or is there going to be separate development between 1.11 and 1.12 for a while?

kashike · 2017-05-10T21:52:05Z

@Cybermaxke I'd say it's better to target 1.12 - there are changes there for 1.12, not in bleeding, and this PR targets 1.12.

stephan-gh · 2017-05-11T18:59:51Z

@Cybermaxke You can easily change the target branch again once we merge 1.12. Until it is merged it should target 1.12 so you get all the updated dependencies etc. (you will also not be able to implement it by targeting bleeding)

killjoy1221 · 2017-05-15T20:14:53Z

src/main/java/org/spongepowered/api/advancement/AdvancementType.java

+ * Changes the frame around the {@link Advancement} icon and
+ * also the appearance in the notifications.
+ */
+public interface AdvancementType extends CatalogType {


@ CatalogedBy(AdvancementTypes.class)

killjoy1221 · 2017-05-15T20:16:47Z

src/main/java/org/spongepowered/api/advancement/AdvancementTypes.java

+
+    /**
+     * Notification appearance:
+     *      Challenge Complete!


Add a method for this in AdvancementType

Something like List<Text> format(Advancement adv);

Should it be a List? Or maybe just a Text object where the lines are separated with \n.

Do not use embedded special characters. Collection where Collection is List, Set, etc.. is the right approach. Please, no magic format characters in the data. Use the Library.

killjoy1221 · 2017-05-15T20:18:42Z

src/main/java/org/spongepowered/api/advancement/criteria/AdvancementCriterion.java

+     * @param criteria The other criteria
+     * @return The and operation
+     */
+    static AdvancementCriterion buildAnd(AdvancementCriterion... criteria) {


You can make it static AdvancementCriterion and(AdvancementCriterion adv1, AdvancementCriterion... therest)

killjoy1221 · 2017-05-15T20:19:58Z

src/main/java/org/spongepowered/api/advancement/criteria/trigger/TriggerType.java

+/**
+ * Represents a source that can trigger a {@link AdvancementCriterion}.
+ */
+public interface TriggerType extends CatalogType {


@ CatalogedBy(TriggerTypes.class)

liach · 2017-05-17T00:43:45Z

src/main/java/org/spongepowered/api/advancement/AdvancementProgress.java

+     * @param criterion The score criterion
+     * @return The score criterion progress
+     */
+    default ScoreCriterionProgress tryGet(ScoreAdvancementCriterion criterion) {


getUnchecked?

Maybe require similar to the data methods?
https://github.com/SpongePowered/SpongeAPI/blob/bleeding/src/main/java/org/spongepowered/api/data/value/ValueContainer.java
getUnchecked isn't used anywhere else as far I know,

stephan-gh · 2017-06-10T11:06:34Z

Please rebase your PR on the latest bleeding branch.

kashike · 2017-06-17T01:12:51Z

@Cybermaxke Are you working on rebasing this?

Cybermaxke · 2017-06-17T01:51:51Z

I will try to get back on this in a week or so, then I will have more time. I will also start implementing at that time.

randombyte-developer · 2017-11-28T20:37:40Z

What's the status on this?

Cybermaxke · 2017-12-04T19:07:32Z

@randombyte-developer I am going to start with the implementation, I already updated the API. ;)

parlough · 2017-12-04T19:08:31Z

@Cybermaxke The hero we all want but don't deserve.

ImMorpheus · 2017-12-05T18:56:45Z

src/main/java/org/spongepowered/api/advancement/criteria/trigger/TriggerTypes.java

+
+    public static final TriggerType CONSUME_ITEM = DummyObjectProvider.createFor(TriggerType.class, "CONSUME_ITEM");
+
+    public static final TriggerType CURSE_ZOMBIE_VILLAGER = DummyObjectProvider.createFor(TriggerType.class, "CONSUME_ITEM");


"CONSUME_ITEM" again ?

Typically, we use labels like Dummy to mean something that does absolutely nothing. But here you're using it to do something meaningful. Why not maker ObjectProvider generic FooObjectProvider seems to be a factory. See also, Design Patterns, GoF. etc..

ImMorpheus · 2017-12-05T19:08:44Z

src/main/java/org/spongepowered/api/advancement/criteria/trigger/TriggerTypes.java

+
+    public static final TriggerType VILLAGER_TRADE = DummyObjectProvider.createFor(TriggerType.class, "VILLAGER_TRADE");
+
+    public static final TriggerType CHANGED_DIMENSION = DummyObjectProvider.createFor(TriggerType.class, "CHANGED_DIMENSION");


This should be before CONSTRUCT_BEACON.

All of these string labels "VILLAGER_TRADE" should be moved into a class holding static final strings. I am loathe to see embedded literal strings in code as much as possible.

just run sortfield once more and it will be fine.

@sibomots The strings here are used only for debugging. the dummy objects are nearly useless and as far as I'm aware don't even necessarily need correct names outside of logging in the case that something goes terribly wrong.

If Java had a way of copying the variable name or line number into a parameter that'd be the ideal solution.

Yes the dummy objects are a gross hack, but it's a gross hack that was decided on by the core team a long while ago, and isn't the subject of this PR outside correcting the incorrect debug text.

ImMorpheus · 2017-12-05T19:13:16Z

Some triggers are missing, is that on purpose ? (effects_changed, levitation, ecc.)

Cybermaxke · 2017-12-06T22:18:31Z

@ImMorpheus No, that's not intentional, they probably added more triggers after I designed the API.

sibomots · 2017-12-06T22:27:33Z

AND, OR.. but not NOT ?

It's also remarkable how many bugs you might find and defects uncover by elaborating the JavaDocs. Please attempt to flesh those out and scratch off the work item "java docs can be improved."

Ie., writing the docs will uncover some defects and deliver aid for those who want to understand the PR.

Cybermaxke · 2017-12-07T14:23:49Z

@sibomots AND and OR operations are the only ones supported in vanilla minecraft. I already added a custom one for a "score" criterion. Do you know a usecase where a NOT operation could be used, this would complicate the internals a lot more so I will only added it if it's worth adding.

I will also document the javadocs a bit more when the implementation is finished, since things can still change.

Cybermaxke · 2017-12-25T02:06:43Z

@XakepSDK Ah yes, that is possible. The example should give a good idea how to do it.

Cybermaxke · 2017-12-25T02:07:09Z

https://github.com/Cybermaxke/SpongeCommon/blob/e055c6e6c6f3c3b0b7e768e6a25bf1bfbbcd7b82/testplugins/src/main/java/org/spongepowered/test/AdvancementTest.java

XakepSDK · 2017-12-25T02:18:18Z

For example - dig dirt. Where do you say, that player should "dig" dirt? Only icon differs. Does advancement icon means action? E.g. shovel = dig, furnace = smelt?

Cybermaxke · 2017-12-25T10:32:24Z

@XakepSDK You trigger the advancements by listening to events for specific actions, for example within the break block event, join event, etc. You have to trigger them by manually. The icon is just visual, it has nothing to do with the triggering

Cybermaxke · 2017-12-27T10:46:02Z

src/main/java/org/spongepowered/api/advancement/AdvancementTree.java

+         * @param id The identifier
+         * @return The advancement tree
+         */
+        AdvancementTree build(String id);


@gabizou Should I move to id to a separate method, to be consistent with the keys builder you added?

…d from trigger, it's not accessible in this form.

- Don't use a specific collection type in the api - Moved events to inner classes - Replaced the advancement tree layout transformer with a event, this is the only moment that you can modify the layout. - Minor changes.

…hod.

…API. Be consistent with added key builder.

gabizou · 2017-12-21T10:22:42Z

src/main/java/org/spongepowered/api/GameRegistry.java

@@ -222,7 +223,10 @@
     *      is not supported
     * @throws CatalogTypeAlreadyRegisteredException if the type cannot be
     *      registered because a matching type was already registered
+     * @deprecated Is scheduled to be removed in API 8, the
+     *      {@link GameRegistryEvent.Register} should be used instead


Nested classes will not link properly in Java 8's javadoc generator. They need to be fully qualified to work.

Signed-off-by: Gabriel Harris-Rouquette <gabizou@me.com>

stephan-gh added the api: 7 (u) version: 1.12 label May 10, 2017

stephan-gh reviewed May 10, 2017

View reviewed changes

kashike suggested changes May 10, 2017

View reviewed changes

killjoy1221 reviewed May 15, 2017

View reviewed changes

liach reviewed May 29, 2017

View reviewed changes

Cybermaxke changed the base branch from bleeding to 1.12 May 31, 2017 22:27

Cybermaxke force-pushed the feature/advancements branch from c441d18 to 45474d0 Compare May 31, 2017 22:29

stephan-gh force-pushed the 1.12 branch 2 times, most recently from 7478cad to 401b69a Compare June 9, 2017 08:50

stephan-gh changed the base branch from 1.12 to bleeding June 10, 2017 11:05

Valandur mentioned this pull request Jun 29, 2017

Load error - Version 3.5.1-S7.0 – Sponge 1.12-7.0.0-BETA-280 Valandur/Web-API#12

Closed

kashike mentioned this pull request Sep 27, 2017

APIs we should look into adding to Sponge over time for our own implementation purposes LanternPowered/Lantern#41

Open

10 tasks

Cybermaxke force-pushed the feature/advancements branch from 6c48a4f to 1be6ae1 Compare December 4, 2017 18:43

Cybermaxke requested a review from gabizou as a code owner December 4, 2017 18:43

ImMorpheus reviewed Dec 5, 2017

View reviewed changes

Cybermaxke commented Dec 27, 2017

View reviewed changes

Cybermaxke force-pushed the feature/advancements branch from 49102ff to cd34a08 Compare December 27, 2017 12:24

Cybermaxke mentioned this pull request Dec 27, 2017

Advancements implementation SpongePowered/SpongeVanilla#363

Merged

Cybermaxke changed the title ~~[WIP] Advancements API~~ Advancements API Dec 27, 2017

parlough added status: accepted priority: high High priority labels Dec 31, 2017

Cybermaxke added 15 commits December 31, 2017 13:54

Added the advancements API

3202756

Moved operator criteria to separate interfaces. Remove dataview metho…

365f794

…d from trigger, it's not accessible in this form.

Changes for compatibility with vanilla mc. Simplify for impl.

157e0fe

More work.

4631081

Updated the events, some improvements

6488efc

- Don't use a specific collection type in the api - Moved events to inner classes - Replaced the advancement tree layout transformer with a event, this is the only moment that you can modify the layout. - Minor changes.

Fixed the SpongeEventFactoryTest for the added events.

b78c567

Progress. Added a game registry event.

8080288

Added generic events.

d57a9e9

Fixed the SpongeEventFactoryTest.

556be61

Make a Advancement extend TextRepresentable, moved the toast text met…

652287b

…hod.

Added a getTextFormat method to the advancement type.

1111bb7

Work on custom triggers.

cdae096

Derp

3fc7cb4

Only one trigger per criterion.

f5635e0

Moved the arguments out of the build methods to be consistent in the …

4124309

…API. Be consistent with added key builder.

Cybermaxke force-pushed the feature/advancements branch from c0b3bf5 to 4124309 Compare December 31, 2017 12:54

gabizou approved these changes Dec 31, 2017

View reviewed changes

gabizou mentioned this pull request Dec 31, 2017

Change entity spawning return types, breaking change - Fixes #1701 #1705

Merged

gabizou added a commit that referenced this pull request Dec 31, 2017

Merge Advancements API. Merges #1554.

34c1fba

Signed-off-by: Gabriel Harris-Rouquette <gabizou@me.com>

gabizou merged commit 4124309 into SpongePowered:bleeding Dec 31, 2017


		public static final TriggerType CONSUME_ITEM = DummyObjectProvider.createFor(TriggerType.class, "CONSUME_ITEM");

		public static final TriggerType CURSE_ZOMBIE_VILLAGER = DummyObjectProvider.createFor(TriggerType.class, "CONSUME_ITEM");


		public static final TriggerType VILLAGER_TRADE = DummyObjectProvider.createFor(TriggerType.class, "VILLAGER_TRADE");

		public static final TriggerType CHANGED_DIMENSION = DummyObjectProvider.createFor(TriggerType.class, "CHANGED_DIMENSION");

Advancements API #1554

Advancements API #1554

Conversation

Cybermaxke commented May 10, 2017 • edited

Choose a reason for hiding this comment

Cybermaxke May 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashike commented May 10, 2017

Choose a reason for hiding this comment

Cybermaxke commented May 10, 2017 • edited

kashike commented May 10, 2017

stephan-gh commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cybermaxke May 30, 2017 • edited

Choose a reason for hiding this comment

stephan-gh commented Jun 10, 2017

kashike commented Jun 17, 2017

Cybermaxke commented Jun 17, 2017

randombyte-developer commented Nov 28, 2017

Cybermaxke commented Dec 4, 2017

parlough commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ImMorpheus Dec 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryantheleach Dec 11, 2017 • edited

Choose a reason for hiding this comment

ImMorpheus commented Dec 5, 2017

Cybermaxke commented Dec 6, 2017

sibomots commented Dec 6, 2017 • edited

Cybermaxke commented Dec 7, 2017

Cybermaxke commented Dec 25, 2017

Cybermaxke commented Dec 25, 2017

XakepSDK commented Dec 25, 2017 via email

Cybermaxke commented Dec 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cybermaxke commented May 10, 2017 •

edited

Cybermaxke May 10, 2017 •

edited

Cybermaxke commented May 10, 2017 •

edited

Cybermaxke May 30, 2017 •

edited

ImMorpheus Dec 5, 2017 •

edited

ryantheleach Dec 11, 2017 •

edited

sibomots commented Dec 6, 2017 •

edited