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

Refactor out some reflective constructor calls #860

Merged
merged 27 commits into from Mar 17, 2018

Conversation

Projects
None yet
4 participants
@Pr0methean
Contributor

Pr0methean commented Mar 8, 2018

Should speed up:

  • chunk loading with entities
  • tree generation
  • spawning entities
  • writing NBT with list tags

Also removes unused imports globally.

Pr0methean added some commits Mar 8, 2018

Refactor out some reflective constructor calls
Should improve performance when generating trees or loading saved chunks
that contain entities.
Switch to ConcurrentHashMap
WeakHashMap won't prevent permgen leaks.

@Pr0methean Pr0methean force-pushed the Pr0methean:removeReflection branch from 426d0f5 to d70bf6d Mar 8, 2018

@Pr0methean Pr0methean requested a review from mastercoms Mar 8, 2018

if (TNTPrimed.class.isAssignableFrom(clazz)) {
entity = new GlowTntPrimed(location, null);
return new GlowTntPrimed(location, null);

This comment has been minimized.

@momothereal

momothereal Mar 8, 2018

Member

This prevents the events from being called (under this statement) for Primed TNT.

This comment has been minimized.

@Pr0methean

Pr0methean Mar 10, 2018

Contributor

Refactored in d78caf8 by overloading the GlowTntPrimed constructor so that it no longer needs separate logic.

} catch (ReflectiveOperationException e) {
throw new IllegalArgumentException("Unable to create list of type " + type, e);
public <V> void putList(String key, TagType type, List<V> value,
Function<? super V, ? extends Tag> tagCreator) {

This comment has been minimized.

@momothereal

momothereal Mar 8, 2018

Member

Why not store the method reference in the enum?

This comment has been minimized.

@Pr0methean

Pr0methean Mar 10, 2018

Contributor

That would create type-safety issues, because an enum can't be generic, so I'm creating put*List wrappers instead.

.put(ZombieVillager.class, GlowZombieVillager.class)
.build();
ENTITIES;
private static ImmutableBiMap.Builder<Class<? extends Entity>, Class<? extends GlowEntity>>

This comment has been minimized.

@smartboyathome

smartboyathome Mar 11, 2018

Contributor

It doesn't appear that we're actually taking advantage of the BiMap here, does it actually need to stay a BiMap? Also, since we already have a CustomEntityContext, I think we should create a BuiltinEntityDescriptor that encapsulates this and entityCtors. Doing this should allow us to move these builders into the static block rather than having them dangle off of the class.

*/
public static GlowEntity constructEntity(Class<? extends Entity> type, Location location) {
if (ENTITY_CTORS.containsKey(type)) {
return ENTITY_CTORS.get(type).apply(location);

This comment has been minimized.

@smartboyathome

smartboyathome Mar 11, 2018

Contributor

Actually, I just realized from the below line that EntityStore is essentially a factory for entities, with its createEntity() method. We should just use that rather than duplicating that here, then custom entities will be handled the same as builtins.

This comment has been minimized.

Pr0methean added some commits Mar 12, 2018

Address #860 (comment)
EntityStorage will now be expected to create new entities for
GlowWorld.spawn(*) by deserializing them from an empty compound tag.

mastercoms and others added some commits Mar 17, 2018

@mastercoms mastercoms merged commit 3e9316a into GlowstoneMC:dev Mar 17, 2018

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment