Registry (For discussion only) #3

dredhorse · 2014-06-20T12:56:19Z

Ok, here is my idea for a way to handle BlockMaterials, ItemMaterials, EntityMaterials etc.

Atm only BlockMaterials is done, and I didn't create any abstract classes or interfaces really.

Most code is in api/material, api/material/block and engine/registry

The plugin running the game will create a registry and pass the kind of registry it want's to use (yaml etc) to it. It will than initialize the registry which will normally load it if the registry is already on file and if it is a server.

Loading the registry means that the mapping ID = MaterialName is done, nothing more, after that the materials need to be added still.

Base / Fallback material can only be used from the plugin holding the registry, custom material can come from other plugins.

you can remove a custom blockMaterial in which case we will fallback to an older version or to the base material.

Client handshake will handle this with a specific packet which will send over the registry to the client, after that the client will need to start adding the blockMaterials.

I guess implementing server-client-handshake will require some rework in the init and check init part, we should only init the registry if the yamlregistry for example is already transmitted.

Feedback?????

Wolf480pl · 2014-06-20T13:52:44Z

What are EntityMaterials? 😜

dredhorse · 2014-06-20T13:58:40Z

well... The stuff which describes the entity... So a tiny creeper or similar...

This allows saving of positions of entities when the server is shut down.

It could be just an id to name mapping in the end.

Wolf480pl · 2014-06-20T14:00:56Z

I thought they were called EntityPrefabs...
And as far as I can tell, saving positions has nothing to do with a registry.

dredhorse · 2014-06-20T14:01:56Z

it may the prefabs than... Well you need to know the kind of prefab you want to save.

And also I want to support changing the prefabs on block, same like with blockMaterial

Wolf480pl · 2014-06-20T14:06:56Z

The plan with EntityPrefabs was that they are used only to create an entity - do tell what components are attached at the beginning. Later, only the components matter. You can attach or detach them at will.
Anyway, we're getting little off-topic. We can continue that on IRC if you want.
Later, I'll have to look at the actual changes you made instead of just complaining about the PR description ;)

dredhorse · 2014-06-20T14:08:27Z

well, we would need to take a look than where we would include the Registry in that process than... And yeah... Also take a look at the PR :-P

kitskub · 2014-06-20T18:05:33Z

@dredhorse I'm having a hard time seeing the actual changes because of all the formatting changes? Can you fix it please? Remembers, spaces not tabs.

kitskub · 2014-06-20T18:09:47Z

src/main/java/com/flowpowered/engine/registry/BlockMaterialRegistry.java

+
+	public boolean saveRegistry() {
+		if (Flow.getPlatform().isClient()) {
+			Flow.debug("Won't be saving registry on client platform!");


This should just throw an error. We don't want to even call this on the client.

Also, do !isServer, because Singleplayer is both.

will change that... It could be possible that we run into problems afterwards though when we need to code the client and server code handling if we run both in one instance... I wonder what the impact would be to run a server and a client instance.

kitskub · 2014-06-20T18:10:58Z

Also, what are the speed implications of using an Attribute system versus fields?

dredhorse · 2014-06-20T18:14:09Z

you mean because of the generic object I'm using? No idea... We could switch it to something else.

I know singleplayer is both, but won't it be 2 instances? Otherwise we should get a both option or isSinglePlayer.

kitskub · 2014-06-20T19:04:26Z

@dredhorse I'd just like to see some kind of profiling so I/we can judge this on that. SIngleplayer is not two instances. The methods are there only as a shortcut for doing two checks (ex. if Client then you have to check if CLIENT or if SINGLEPLAYER)

Wolf480pl · 2014-06-21T20:09:32Z

src/main/java/com/flowpowered/engine/registry/BlockMaterialRegistry.java

+
+	/**
+	 * Class of the instancing Plugin.
+	 * This is required to make sure that only objects from the instancing game can be used as fallback objects.


No it doesn't !
I mean... maybe it is required in order to , but we don't want to .

you need too, fallback material must be something that ALL plugins for that "game" understand, otherwise any plugin can register a fallback material (why honestly???) and other plugins don't understand that and will generate errors... and if your "game" support also an external client that client will either crash or get blocks he can't display.

Even if we do need it, comparing package name is NOT the way to go.
Also, this should be (able to be) delegated to a class that extends BlockMaterialRegsitry. This is policy. We should separate policy from mechanisms.

Wolf480pl · 2014-06-21T20:42:15Z

Other random notes:

Getting unique names of materials from their class names isn't ideal, but we can easily change that.
Why do you need all this ~~crap~~ stuff in the TempPlugin classes? You're not using all these methods.
The Attribute.getInstance() method kinda suggests (by means of its name) that it would cache attribute instances somehow, it doesn't though. Dunno if it's desired.
Also, you have some redundant checks ~~in addCustomBlockBaseMaterial().~~ everywhere!
Oh, and you're unnecessarily setting the attribute to the changed list that you just got from it, because you never actually copy the list. So if you get(CUSTOM_MATERIAL).add(blockBaseMaterial), then the list in the attribute already contains what you added, and you don't need to set the attribute.

Wolf480pl · 2014-06-21T21:14:34Z

Also, one major problem you didn't solve:
The BaseMaterial.getID() returns an ID assigned by the old registry, not the new one.

dredhorse · 2014-06-22T08:31:01Z

we need this crap in TempPlugin so that other plugins are able to grab the Registries when the want to make changes... Where else do you want to store that information, as discussed there will not be by default a Registry per World approach.

Can cut down on the redundant checks.

Will check the get.add attribute stuff.

What do you exactly mean by the BaseMaterial.getID() returns an ID assigned by the old registry?

Wolf480pl · 2014-06-22T10:17:41Z

By crap in TempPlugin I mean onClientEnable, setEnabled, etc. Stuff that a plugin would nomally have, but you do NOT NEED it in order to implement the registry.

Wolf480pl · 2014-06-22T10:19:26Z

src/main/java/com/flowpowered/api/material/BaseMaterial.java

+	public BaseMaterial(String name) {
+		this.displayName = name;
+		this.name = getClass().getCanonicalName() + "_" + name.replace(' ', '_');
+		this.id = (short) MaterialRegistry.register(this);


By getID returns ID from old registry I mean exactly this...

Ah… Mistake from my side… Thanks..

On 22.06.2014, at 12:19, Wolf480pl [email protected] wrote:

In src/main/java/com/flowpowered/api/material/BaseMaterial.java:

private final String name;

private String displayName;

private int maxStackSize = 64;

private MATERIAL_STATE materialState = MATERIAL_STATE.UNDEFINED;

public static enum MATERIAL_STATE {

SOLID, LIQUID, GAS, UNDEFINED

}

/**

* Creates a material with a name

*/

public BaseMaterial(String name) {

this.displayName = name;

this.name = getClass().getCanonicalName() + "_" + name.replace(' ', '_');

this.id = (short) MaterialRegistry.register(this);
By getID returns ID from old registry I mean exactly this...

—
Reply to this email directly or view it on GitHub.

dredhorse · 2014-06-22T10:20:19Z

Ah, ok… Well I did just put that stuff in as it was missing atm… I know it will be done it it’s own project. So ignore that atm..

On 22.06.2014, at 12:17, Wolf480pl [email protected] wrote:

By crap in TempPlugin I mean onClientEnable, setEnabled, etc. Stuff that a plugin would nomally have, but you do NOT NEED it in order to implement the registry.

—
Reply to this email directly or view it on GitHub.

kitskub · 2014-06-24T16:21:55Z

@Wolf480pl @dredhorse for Attribute.getInstance() method, I think it should just be renamed to Attribute.get(). It provides an aesthetic shortcut to new Attribute<G, T>()

kitskub · 2014-06-24T16:59:51Z

@dredhorse the Attribute system looks similar to the defaulted package in flow-commons?

dredhorse · 2014-06-24T18:22:56Z

Hmm…. Need to check that

On 24.06.2014, at 18:59, Jack Huey [email protected] wrote:

@dredhorse the Attribute system looks similar to the defaulted package in flow-commons?

—
Reply to this email directly or view it on GitHub.

dredhorse · 2014-06-29T10:15:52Z

did you mean the defaultedHashMap?

kitskub · 2014-06-29T16:07:54Z

@dredhorse yes

dredhorse · 2014-07-10T18:05:16Z

will look into that again... but I guess we can always fallback to normal objects if we have an issue later on.

I will go on a 2 week vacation and will try to finish this, and also squash the commits to make it easier to review.

We can than decide if we pull it or not.

kitskub · 2014-08-06T23:42:29Z

@dredhorse What's up with this? :D

Also, be sure to rebase.

dredhorse · 2014-08-07T13:51:34Z

Working on it, if you mean when will it be done.

BTW: Also found out that we probably need a registry for world generators incl. Versions.

On 07.08.2014, at 01:42, Jack Huey [email protected] wrote:

@dredhorse What's up with this? :D

—
Reply to this email directly or view it on GitHub.

dredhorse added 6 commits February 13, 2014 12:43

Created new branch registry

9514245

Initial commit for new BlockRegistry

71a68e8

adding temp plugin classes

b117b73

changed BlockMaterial to correctly support generics

4580fa7

added RegistryNotInitializedException

1571e4b

refactored to support yamlregistry object.

b43e1b7

kitskub reviewed Jun 20, 2014
View reviewed changes

Wolf480pl reviewed Jun 21, 2014
View reviewed changes

Wolf480pl reviewed Jun 22, 2014
View reviewed changes

refactored to support yamlregistry object.

26f0699

kitskub force-pushed the master branch 3 times, most recently from 1a624be to 011d9ab Compare September 14, 2014 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Registry (For discussion only) #3

Registry (For discussion only) #3

dredhorse commented Jun 20, 2014

Wolf480pl commented Jun 20, 2014

dredhorse commented Jun 20, 2014

Wolf480pl commented Jun 20, 2014

dredhorse commented Jun 20, 2014

Wolf480pl commented Jun 20, 2014

dredhorse commented Jun 20, 2014

kitskub commented Jun 20, 2014

kitskub Jun 20, 2014

dredhorse Jun 29, 2014

kitskub commented Jun 20, 2014

dredhorse commented Jun 20, 2014

kitskub commented Jun 20, 2014

Wolf480pl Jun 21, 2014

dredhorse Jun 22, 2014

Wolf480pl Jun 22, 2014

Wolf480pl commented Jun 21, 2014

Wolf480pl commented Jun 21, 2014

dredhorse commented Jun 22, 2014

Wolf480pl commented Jun 22, 2014

Wolf480pl Jun 22, 2014

dredhorse Jun 22, 2014

dredhorse commented Jun 22, 2014

kitskub commented Jun 24, 2014

kitskub commented Jun 24, 2014

dredhorse commented Jun 24, 2014

dredhorse commented Jun 29, 2014

kitskub commented Jun 29, 2014

dredhorse commented Jul 10, 2014

kitskub commented Aug 6, 2014

dredhorse commented Aug 7, 2014

Registry (For discussion only) #3

Are you sure you want to change the base?

Registry (For discussion only) #3

Conversation

dredhorse commented Jun 20, 2014

Wolf480pl commented Jun 20, 2014

dredhorse commented Jun 20, 2014

Wolf480pl commented Jun 20, 2014

dredhorse commented Jun 20, 2014

Wolf480pl commented Jun 20, 2014

dredhorse commented Jun 20, 2014

kitskub commented Jun 20, 2014

kitskub Jun 20, 2014

Choose a reason for hiding this comment

dredhorse Jun 29, 2014

Choose a reason for hiding this comment

kitskub commented Jun 20, 2014

dredhorse commented Jun 20, 2014

kitskub commented Jun 20, 2014

Wolf480pl Jun 21, 2014

Choose a reason for hiding this comment

dredhorse Jun 22, 2014

Choose a reason for hiding this comment

Wolf480pl Jun 22, 2014

Choose a reason for hiding this comment

Wolf480pl commented Jun 21, 2014

Wolf480pl commented Jun 21, 2014

dredhorse commented Jun 22, 2014

Wolf480pl commented Jun 22, 2014

Wolf480pl Jun 22, 2014

Choose a reason for hiding this comment

dredhorse Jun 22, 2014

Choose a reason for hiding this comment

dredhorse commented Jun 22, 2014

kitskub commented Jun 24, 2014

kitskub commented Jun 24, 2014

dredhorse commented Jun 24, 2014

dredhorse commented Jun 29, 2014

kitskub commented Jun 29, 2014

dredhorse commented Jul 10, 2014

kitskub commented Aug 6, 2014

dredhorse commented Aug 7, 2014