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

1.13 - The Technical and Aquatic Update #1704

Open
wants to merge 2 commits into
base: bleeding
from

Conversation

@Cybermaxke
Copy link
Member

Cybermaxke commented Nov 27, 2017

Very WIP, I am opening this PR now to start a discussion on what can be reused/needs to be removed. Also designing a system to easily get variants of certain block types. For example getting a slab with the material oak.

Related to:
#1715 - The variant system
LanternPowered/LanternServer#48
#1742

We have to decide what to do with the following data types, could it be reused for the variant system or should be deleted. I already removed a few types, feel free to comment if you don't agree with me removing them.

Modify/remove:

Catalog types and data:

  • BrickTypes - Removed
  • BrickData - Removed
  • CoalTypes - Removed
  • CoalData - Removed
  • CookedFishes - Removed
  • CookedFishData - Removed
  • DirtTypes - Removed
  • DirtData - Removed
  • DisguisedBlockTypes - Removed
  • DisguisedBlockData - Removed
  • DoublePlantTypes - Removed
  • DoublePlantData - Removed
  • DyeColors - Still used by other data types, used in variant system
  • DyeableData - Still used by other data types
  • Fishes - Removed
  • FishData - Removed
  • LogAxes - Removed, bark got separated from log, log uses now the Axis
  • LogAxisData - Removed
  • GoldenApples - Removed
  • GoldenAppleData - Removed
  • PistonTypes - Removed
  • PistonData - Removed
  • PlantTypes - Removed
  • PlantData - Removed
  • PrismarineTypes - Removed
  • PrismarineData - Removed
  • SandstoneTypes - Removed
  • SandstoneData - Removed
  • SandTypes - Removed
  • SandData - Removed
  • ShrubTypes - Removed
  • ShrubData - Removed
  • SkullTypes - Removed
  • SkullData - Removed
  • SlabTypes - Removed
  • SlabData - Removed
  • StoneTypes - Removed
  • StoneData - Removed
  • TreeTypes - Will be used in the variant system
  • TreeData - Removed
  • WallTypes - Removed
  • WallData - Removed
  • QuartzTypes - Removed, separate blocks, pillar variant now uses the Axis.
  • QuertzData - Removed
  • BigMushroomTypes - Removed, replaced with BigMushroomPoresData
  • BigMushroomData - Removed, replaced with BigMushroomPoresData

Other data (and keys):

  • SeamlessData - Removed
  • SpawnableData

Tile entities:

  • Note - Removed
  • Skull - Removed
  • FlowerPot- Removed

New/Modified

Data (probably separate PRs):

  • LitData - New (furnaces, redstone torch, redstone)
  • InstrumentData - New (note block)
  • SlabPortionData (and SlabPortion) - New
  • ChestAttachmentData (and ChestAttachmentType) - New
  • InvertibleData - New (daylight sensor)
  • SurfaceAttachmentData (floor, ceiling, wall) - New (buttons, lever), is this a good name?
  • BigMushroomPoresData - New (replaces BigMushroomData)
  • UnstableData - New (TNT)

Tile entities:

  • Player head - New

Populators:

  • Merged Shrub and Flower into Plant

Other:

  • Update statistics, moved to #1742
  • Update block traits
  • Update particles
  • RecordType -> MusicDisc - In 1.13, the name 'record' is no longer used in any translation keys, item ids, etc. (item names were already Music Discs)
  • Remove extended block states? Everything is now controlled server side. - Forge is still going to use them.

@Cybermaxke Cybermaxke requested review from Faithcaio and gabizou as code owners Nov 27, 2017

@VapidLinus

This comment has been minimized.

Copy link

VapidLinus commented Nov 27, 2017

Just leaving a quick note that this PR seems to relate to my issue regarding how to get a coloured ShulkerBox from a DyeColor: #1667

edit: music disc type, maybe?

@me4502

This comment has been minimized.

Copy link
Member

me4502 commented Nov 27, 2017

I’m of the opinion it’s still nice to have groupings of obvious variants, such as wool colours. Idk how others feel tho

@ryantheleach

This comment has been minimized.

Copy link
Member

ryantheleach commented Nov 27, 2017

Before you get too far into this, remember that the Data API is an abstraction over the base game.

Having this data available, even if not directly represented in the Mojang block properties was entirely the point of abstracting it in the first place.

BlockTrait API on the other hand, was supposed to be a closer to metal abstraction, that was implementation dependent.

@Cybermaxke

This comment has been minimized.

Copy link
Member

Cybermaxke commented Nov 27, 2017

@ryantheleach That's why I am opening a discussion on this. And lot (block) based data is still tied quite closely to the MC internals, making it not possible to update without breaking changes (or ugly workarounds). Since there are already a lot BlockType changes, I don't see any issues with breaking the data types as well.

@Cybermaxke

This comment has been minimized.

Copy link
Member

Cybermaxke commented Nov 27, 2017

@VapidLinus There is a RecordType catalog type.

@Cybermaxke Cybermaxke changed the base branch from 1.13 to bleeding Dec 4, 2017

@Cybermaxke Cybermaxke force-pushed the LanternPowered:1.13 branch from 34a5ee7 to d8fca68 Dec 4, 2017

@gabizou
Copy link
Member

gabizou left a comment

Some good ideas, but I think I've found a direction I'd like to take this towards.

Overall, I'm going to still have a think about Variety and Varieties to define what you're calling Material. Effectively what it should solve for is defining a Wool Carpet Block as having Varieties.WOOL, Varieties.CARPET, and Varieties.BLUE to describe the block. This way you can determine already that it is a wool block, it's a carpet (so you can gather all blocks that are matching Varieties.CARPET and WOOL, or simply attempt to match all blocks/items with WOOL varieties.

It would be interesting to try and resolve the awkwardness of showing DyeColor as a value instead of a Property of sorts. One way this is retrievable should be through the existing Property API system in place with Data API. Since it is not an immutable data (can't have mutable properties), you can retrieve the whole collection of Varieties in this fashion.

Would love some input from @SpongePowered/developers .

* applicable to {@link BlockTypes#QUARTZ_BLOCK}.
*/
public interface ImmutableQuartzData extends ImmutableVariantData<QuartzType, ImmutableQuartzData, QuartzData> {
public interface Material extends CatalogType {

This comment has been minimized.

@gabizou

gabizou Dec 7, 2017

Member

After thinking about it more today, I don't like the idea of naming it Material. Maybe call it Variety and then Varieties.OAK and Varieties.QUARTZ could then work. This way you can also have Varieties.STAIR or PLANK even. This way the "type" is not defined as a Material (since stair is not a material, but a variety of a block). This also allows for us to make a combination of varieties for a specific object to "describe" it.

Thinking on it more, Variety could even be introduced in API 7 (given some time this week maybe to solidify it as a possibility), in which a "moving" towards it is somewhat available before the drastic change of some of this data removal.

Thoughts @Zidane ?

@@ -207,6 +207,8 @@

public static final SoundType BLOCK_PORTAL_TRIGGER = DummyObjectProvider.createFor(SoundType.class, "BLOCK_PORTAL_TRIGGER");

public static final SoundType BLOCK_PUMPKIN_CARVE = DummyObjectProvider.createFor(SoundType.class, "BLOCK_PUMPKIN_CARVE");

This comment has been minimized.

@gabizou

gabizou Dec 7, 2017

Member

Carve or Carved?

This comment has been minimized.

@JBYoshi

JBYoshi Dec 9, 2017

Member

Carve. This is the sound that plays when an uncarved pumpkin turns into a carved pumpkin.


@CatalogedBy(LogAxes.class)
public interface LogAxis extends CatalogType, Cycleable<LogAxis> {
public interface VariantQueryType<T> extends CatalogType {

This comment has been minimized.

@gabizou

gabizou Dec 7, 2017

Member

What's the point of this? It's feeling very much either like a Property or a Key in some aspects.

public static final LogAxis NONE = DummyObjectProvider.createFor(LogAxis.class, "NONE");

public static final LogAxis X = DummyObjectProvider.createFor(LogAxis.class, "X");
public static final VariantQueryType<Material> MATERIAL = DummyObjectProvider.createFor(VariantQueryType.class, "MATERIAL");

This comment has been minimized.

@gabizou

gabizou Dec 7, 2017

Member

With my comment above about Variety vs Material, this is an example where "Variants" will win overall as you can use different varieties to describe something unique (almost mergeable with properties).


import java.util.Optional;

public interface VariantQueryableCatalogType extends CatalogType {

This comment has been minimized.

@gabizou

gabizou Dec 7, 2017

Member

Description of what this type of catalog type does?

* @param <T> The argument type
* @return The argument type, if the query type is supported
*/
<T> Optional<T> getArgument(VariantQueryType<T> queryType);

This comment has been minimized.

@gabizou

gabizou Dec 7, 2017

Member

Why is it a query type instead of just VariantQuery? We don't call Keys KeyType.

This comment has been minimized.

@Cybermaxke

Cybermaxke Dec 7, 2017

Member

It was to return the argument you would pass in to create the VariantQuery from the VariantQueryType.

@Cybermaxke

This comment has been minimized.

Copy link
Member

Cybermaxke commented Dec 7, 2017

@gabizou I will open a new PR that targets API 7 for the Variety system. I will try to reuse things from the Data API that are already present to replace the VariantQuery, like using properties to query for specific things.
TreeTypeProperty, DyeColorProperty, etc.
Which other properties would you think that would be worth adding, based from the data that will be removed, for example:
GoldenAppleTypeProperty, CoalTypeProperty, etc.

@kashike kashike changed the base branch from bleeding to 1.13 Dec 8, 2017

@kashike kashike force-pushed the SpongePowered:1.13 branch from fdc5328 to d7f2e69 Dec 8, 2017

@kashike

This comment has been minimized.

Copy link
Member

kashike commented Dec 8, 2017

Please rebase against 1.13.

@Cybermaxke Cybermaxke force-pushed the LanternPowered:1.13 branch from acb4011 to 7670f60 Dec 9, 2017

@Cybermaxke Cybermaxke requested a review from Deamon5550 as a code owner Dec 9, 2017

*
* @see LitData#lit()
*/
public static final Key<Value<Boolean>> LIT = KeyFactory.fake("LIT");

This comment has been minimized.

@ryantheleach

ryantheleach Dec 11, 2017

Member

🚬🔥

@kashike kashike force-pushed the SpongePowered:1.13 branch 2 times, most recently from 8277fda to d32cbb1 Jan 3, 2018

@Cybermaxke Cybermaxke force-pushed the LanternPowered:1.13 branch 2 times, most recently from 060576b to 06647ca Jan 15, 2018

@gabizou gabizou self-assigned this Jan 15, 2018

@Cybermaxke Cybermaxke referenced this pull request Jan 17, 2018

Open

Minecraft 1.13 - Refactor statistics #1742

0 of 2 tasks complete
@gabizou
Copy link
Member

gabizou left a comment

I’m willing to allow keeping the removed data, only to support retrieving the data, and then removing it later. This way plugins still will have an upgrade understanding. I know that this api is supposed to break the world, but it’d be nice to retain it for support like we did with some data types in api 6.

@Cybermaxke

This comment has been minimized.

Copy link
Member

Cybermaxke commented Feb 15, 2018

@gabizou I don't feel like keeping something as half working is a good idea. Maybe it would be better to provide a spongedocs page dedicated to updating from API7 to API8, related to big changes like item types/block types?

@parlough

This comment has been minimized.

Copy link
Member

parlough commented Feb 15, 2018

@Cybermaxke I think I share the same opinion, keeping it with half functionality will unnecessarily clutter the API and only confuse developers. You are right though we need to document all of the changes well, not only updating outdated docs but a migration guide will probably be highly beneficial. In general we should keep a more detailed changelog of larger changes as well.

@liach

This comment has been minimized.

Copy link

liach commented Mar 20, 2018

Bump: @Cybermaxke Would you add contents from the aquatic update, aside technical changes?

@Cybermaxke

This comment has been minimized.

Copy link
Member

Cybermaxke commented Mar 20, 2018

@liach Yes, I am planning on doing that.

@Cybermaxke Cybermaxke changed the title 1.13 - The Technical Update 1.13 - The Technical and Aquatic Update Mar 20, 2018

@Cybermaxke Cybermaxke force-pushed the LanternPowered:1.13 branch from e02c089 to 4617482 Apr 7, 2018

@Cybermaxke Cybermaxke requested review from kashike , Minecrell and Zidane as code owners Apr 7, 2018

@gabizou

This comment has been minimized.

Copy link
Member

gabizou commented Apr 12, 2018

Can you update the PR description as to what is going on? (I believe we opted to support legacy data, on the basis that it was read only, much like the option we took with HorseVariants when horses got split up.

@gabizou
Copy link
Member

gabizou left a comment

There's a metric ton of formatting changes that I'm not able to see what is actually added/changed in the API without scrolling and reading every line diff (I feel that the first 10th of the PR changes are just formatting....)

@Cybermaxke Cybermaxke changed the base branch from 1.13 to bleeding Apr 12, 2018

@Cybermaxke

This comment has been minimized.

Copy link
Member

Cybermaxke commented Apr 12, 2018

@gabizou I rebased against bleeding but didn't change the target branch, is fixed now.

I still think that keeping half broken API isn't a good thing, and that it would be better to provide good documentation to transition between API 7 and API 8 (once 1.13 is merged).

Would love some input on this from some other developers

@parlough

This comment has been minimized.

Copy link
Member

parlough commented Apr 12, 2018

@Cybermaxke I share your opinion, leaving half functional API would be a pain for maintenance and it will just cause confusion for users. They will have to update and make changes anyway for their plugin to be functional and they can update their removed data retrievals while they're at it. Leaving it will have them expecting API to work that won't work and/or even can't be made to work.

Our best solution is documenting all the necessary changes quite well, through written documentation, but also through video or stream. I am planning to work on this quite a bit and I know others will be happy to help.

@Cybermaxke Cybermaxke force-pushed the LanternPowered:1.13 branch from 47d3ace to 728ce67 May 12, 2018

@Cybermaxke Cybermaxke force-pushed the LanternPowered:1.13 branch from 1e30f69 to fa93d0f May 26, 2018

* @return The immutable value for the persistent state
* @see Keys#PERSISTS
*/
ImmutableValue<Boolean> persists();

This comment has been minimized.

@kashike

kashike Jun 5, 2018

Member

persistent

This comment has been minimized.

@Cybermaxke

Cybermaxke Jun 5, 2018

Member

@kashike I named it persists to match the Key

This comment has been minimized.

@kashike

kashike Jun 5, 2018

Member

name the key persistent, then! :P

@Cybermaxke Cybermaxke force-pushed the LanternPowered:1.13 branch from a057bcf to dcfe5ef Jun 22, 2018

@Cybermaxke Cybermaxke force-pushed the LanternPowered:1.13 branch from f28307a to 7fdf6e1 Sep 20, 2018

@Cybermaxke Cybermaxke force-pushed the LanternPowered:1.13 branch from 7fdf6e1 to be904f8 Sep 20, 2018

@Deamon5550 Deamon5550 removed their request for review Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment