Fix #371: add hook for default base aspect applied to all aspects

[schosin]

This PR adds the ability to add defaults to every aspect unless explicitly excluding those defaults.

I'm not sure about the best way to disable default aspects for All/One/Excude. For now I added the flag to Exclude, but I'm open for alternatives.

---

-- Copied from #371
Here's an overview what I've done:

• Default aspect is configured via WorldConfiguration(Builder), by default applied to all subscriptions
• Aspect.Builder
  • has a flag whether to apply defaults or not
  • has a couple of fields changed to package private for easier access in asm (not too happy about this)
• AspectSubscriptionManager
  • has optional field for a default aspect, set by WorldConfiguration
  • applies the default aspect according to the parameter and builder
• EntityTransmuter explicitly ignores default aspect regardless of the Aspect.Builder
• WorldConfiguration/Builder have optional default aspect
• AspectDescriptor: flag defaults() default true
• Exclude: flag excludeDefaults() default false (not sure this is the best way for All/One/Exclude)
• AspectFieldResolver updated for AspectDescriptor/Exclude
• Updated / added a bunch of tests

This should result in no runtime changes for existing applications and a slight overhead for creations of EntitySubscriptions.
I have only worked in the artemis-core/artemis module. All tests are passing, but I'm not certain whether I forgot anything (plugins, serialization, fluid, gwt, ...).

[schosin]

Somehow I broke the GWT tests, didn't know about the gwttest maven profile. I don't have any experience with GWT, but I'll see what I can do.

[schosin]

👋 @junkdog

[DaanVanYperen]

Thanks for the PR! Sorry for letting it sit so long.
I would probably use this feature myself. @junkdog what do you think? Sufficiently in scope?
(Haven't reviewed the code yet)

[schosin]

I can fix the conflict. Would you rather see a merge or rebase?

Regarding the code review, there is already some discussion in #371. Regarding performance, I wasn't able to get the benchmarks to run on my system, so I never provided any.

[DaanVanYperen]

Performance is probably not an issue, nobody should have aspect creation in a tight loop post init.
Except maybe in serialization somewhere?

[DaanVanYperen]

I can fix the conflict. Would you rather see a merge or rebase?

Leave it, the conflict is so minor we can fix it on merge.

[DaanVanYperen]

While the code is sound I think the API needs some work before we can consider merging.

Just thinking out loud, let me know what your thoughts are:

• This is a power user feature so ideally we keep the core API the same for basic users.
• I'd favor a little more verbosity at aspect annotation sites just to make it obvious the default aspect is going to be applied; @One(..) @All(..) @DefaultAspect (name pending). And if we do that, maybe it makes sense to give the user a little more control /which/ global aspect is applied?
• For programmatic Aspects it might make more sense to just have a Aspect builder Aspect.builder.merge(Globals.DEFAULT) so the way things work are obvious, flexible and less magical.
• The API doesn't document (yet) how odb will combine the global and local aspects and how conflicts are resolved.

[DaanVanYperen]

We can cache this.

[schosin]

Since we already assumed that most people don't create EntitySubscriptions in tight loops (I'd say mostly in the initialize phase), such a cache would increase the memory usage for every distinct aspect used and only helps for those aspects that are used multiple times.

Personally I think I have more unique aspects in my projects than non-unqiue aspects, so it's wasting memory for marginal improvement in startup time, if any.

[DaanVanYperen]

Why is the original code even obtaining a subscription in the first place? It doesn't even do anything with subscriptions.

[schosin]

By doing it that way, the aspect goes through ComponentManager#synchronize. Doesn't look like that's necessary for an EntityTransmuter, but not 100% sure. Could be simplified to aspect.build(world). I would still need a way to exclude defaults for this class.

[DaanVanYperen]

This one is for @junkdog.

[DaanVanYperen]

Clarify what 'to be applied' means and how it resolves conflicts. You could potentially end up with aspects with the same component in both exclude or include.

[schosin]

Looking at your feedback, I agree that we should consider moving the "exclude default" out of @Exclude and @AspectDescriptor into a @ExcludeDefaultAspect.

By having such an annotation, @DefaultAspect doesn't make any sense to me. @DefaultAspect implies that the default aspect won't be applied if it's missing, which defeats the purpose of a global default aspect in my mind. If @DefaultAspect is required, there's no need for @ExcludeDefaultAspect as we can just omit @DefaultAspect instead.

Personally I'd prefer moving those flags to a new annotation @ExcludeDefaultAspect. That would result in no changes for existing users not interested in this feature.

As for "/which/ global aspect", I would also not implement multiple, selectable global aspects using this default aspect mechanic. If we want something like that, I'd rather say we'll look into something like Aspect.Builder#merge(Aspect.Builder) and Spring's meta annotations to quickly add "aspects constants".

@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.FIELD, ElementType.METHOD, ElementType.TYPE })
@All(Alive.class)
@Exclude(Paused.class)
public @interface AliveUnpaused {}

[DaanVanYperen]

Brain dump, lemme know what you think.

Meta annotations

It's an interesting idea, reconstituting aspects into annotation based types, and it works around the limits of constants in annotations. If they stack the use cases go beyond just global aspects.

@Mobs public EntitySubscription mobs;
@Mobs @All(Flying.class) public EntitySubscription flyingMobs;

It has a lot of potential but the idea needs a bit of polish and integration. Worth talking over with @junkdog. Feels like it could fit 3.0.

Only technical blocker is the GWT reflection generator not storing metadata on annotations (it should be doable).

Personally I'd prefer moving those flags to a new annotation @ExcludeDefaultAspect. That would result in no changes for existing users not interested in this feature.

For 2.5 @ExcludeDefaultAspect looks like the way to go. AspectFieldResolver should do the merge for everything (excluding Transformers). At dependency injection time all this info should be available.

For EntitySystem constructor aspects we can merge the defaults in SystemMetadata::getAspect and just check for @ExcludeDefaultAspect on the class.

Users can use Aspect.Builder#merge(Aspect.Builder) with a global constant at any programmatic sites.

We don't do anything magical in subscription manager or anywhere else in the API to keep things predictable; if you make a call with a certain Aspect.Builder you'll get the stuff for that Aspect.Builder.

If the default aspect is not set nothing happens, it becomes zero impact for users. No magic or API or behavioral changes.

--

Big picture, global aspects still feel a bit kitchen-sinky and edge-casey which is something @junkdog wants to avoid. Can we come up with extension point for an AspectFactory so we could put the impl into a plugin?

Edit: changed order a bit.

[schosin]

In general I would say that Aspect.Builder#merge(Aspect.Builder) should behave the same way as we want default aspects to behave. That way we can move the actual merge to that class and document it's behaviour on that method.
The only downside is that the current implementation uses a temporary bag for doing all the work. While I don't think a lot of people are creating aspects concurrently, it's still the safer bet to have a lazily created tmpBag for each Aspect.Builder instance.

Defaults in AspectFieldResolver:
Works for me. If the world has a default aspect, it would be applied using the merge function for Aspect.Builder and EntitySubscription.
I just don't like the idea of mixing EntitySystem constructor builder with an annotation. For consistency's sake we should change the defaults flag on Aspect.Builder to boolean excludeDefaults = false. The constructor call should not be affected by any annotations as it is the case today.

Defaults in SystemMetadata:
SystemMetadata is currently used for the case where the is no Aspect.Builder passed in the constructor, so it's not really the right place in my opinion. Since we don't have access to the World until setWorld is called, there's no means to get access to the default Aspect anyway.
For EntitySubscription it kinda has to stay in AspectSubscriptionManager. After all EntitySubscription get(Aspect.Builder builder) can be called explicitly as well and I'd argue the default aspect should also be applied in those cases (unless excludeDefaults is set).

Thoughts on AspectFieldResolver and Aspect: Why is there no case for creating Aspect? Probably not the most used feature, but maybe someone wants a field to check whether an entity matches another aspect using Aspect#isInterested(Entity).
Although that feature doesn't work well with int entityIds since Aspect has no access to resolving an entity's componentBits currently. Passing the world to it's constructor would allow implementing Aspect#isInterested(int). Maybe for another PR.

As a side note: We should also not forget about TagManager and GroupManager. Adding "Paused" to an entity should not magically remove it's tag or groups.