[FEATURE] Debugging invalid references.

[DaanVanYperen]

as a framework user I want to log when and where entities are created or destroyed. So that I can debug reference issues.

Criteria (edit after discussion)

• Debug plugin
  • Ability to detect illegal access to components and entities.
  • Ability to report the cause (for example, delete issued in class@linenumber).
  • Ability to log all lifecycle events (optional).
  • Expose all debug information to ECS (via components).
  • Lifecycle naming of entities for easy tracking.
  • Extend/replace logging and naming.
  • Plug and play.
• Lifecycle listener plugin.
  • Intended to augment editors and debuggers.
  • Can be disabled. No performance impact when disabled.
  • Hook into lifecycle events for Entities and Components.
  • Hook into lifecycle events for World (?)
  • Plug and play.

Code example

To log all calls coming from net.mostlyoriginal package.

WorldConfigurationBuilder.with(DebugPlugin.thatLogsErrorsIn("net.mostlyoriginal"))

log output (Proof of concept)

NotableNautilus(1) CREATE @ net.mostlyoriginal.game.system.map.TiledMapManager.createLayerEntity(TiledMapManager.java:85)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.load(TiledMapManager.java:73)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.initialize(TiledMapManager.java:62)
NotableNautilus(1) DELETE @ net.mostlyoriginal.game.system.map.TiledMapManager.createLayerEntity(TiledMapManager.java:90)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.load(TiledMapManager.java:73)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.initialize(TiledMapManager.java:62)
*********************
NotableNautilus(1) ERROR_ATTEMPT_TO_DELETE_DELETED_ENTITY @ net.mostlyoriginal.game.system.map.TiledMapManager.createLayerEntity(TiledMapManager.java:91)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.load(TiledMapManager.java:73)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.initialize(TiledMapManager.java:62)
Cause (Already deleted at):
NotableNautilus(1) DELETE @ net.mostlyoriginal.game.system.map.TiledMapManager.createLayerEntity(TiledMapManager.java:90)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.load(TiledMapManager.java:73)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.initialize(TiledMapManager.java:62)
*********************

Background

While attempting to refactor a Jam game into a more best practices example for ECS I'm running into a lot of bugs related to expired id references, even with the help of @EntityId.

[junkdog]

Do we really need to extend the world? A custom SystemInvocationStrategy exposes initialize, setSystems but probably missing hooks for updateEntityStates for it to be convenient.

I don't mind including a logging facade. I find logback to be pretty good. Could use proguard to strip all logging-related code from non-debug artifacts.

An EntitySubListener (or system) for Aspect.all() could supply entities with a secondary id/counter in inserted(Entities).

[DaanVanYperen]

I agree with you, ideally i'd be able to use the Plugin API and not touch world directly. Would be easier to use for the end user and solve some order of operation issues as well. Think it can be done if you don't mind some small changes to WorldConfiguration, specifically adding the ability to supply an extended batchProcessor and entityManager.

Got a PoC running with DebugWorld. Overriding world.deleteto get the stacktrace at the moment user calls for deletion, which is handled by batchProcessor which isn't exposed in WorldConfiguration (yet). For creation calls a custom entitymanager is supplied.

Slap DebugComponent on the entity at creation so user can use it for their own debugging systems, and track it separately so we can always access it even if the world lifecycle hasn't updated.

https://gist.github.com/DaanVanYperen/e2ab0143f2b92e49d064c913c3fc6a02

Don't know about logback for this use case, just got a very basic callback that doesn't assume anything. Proguard for a debug artifact with broader debugging might be coolies?

    public interface DebugLogStrategy {
        void log(String s);

        void log(MutationStacktrace site);
    }

Works pretty great so far, figured out my bug in no time. Coolest thing is the gfycat name gen ;)

AdmiredFallowdeer(0) CREATE @ net.mostlyoriginal.api.SingletonPlugin$SingletonFieldResolver.initialize(SingletonPlugin.java:40)
RustyTinamou(1) CREATE @ net.mostlyoriginal.game.system.map.TiledMapManager.createLayerEntity(TiledMapManager.java:85)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.load(TiledMapManager.java:73)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.initialize(TiledMapManager.java:62)

[junkdog]

Going to bed, already late, (belated hi!), so forgive the brevity - but this was cool, so I had to reply.

Think it can be done if you don't mind some small changes to WorldConfiguration, specifically adding the ability to supply an extended batchProcessor and entityManager.

Hmm, couldn't we provide similar functionality by opening up said classes with the appropriate optional callbacks? It's easier to iterate on the internal design if we can negate the need to extend the classes in the first place, instead providing custom logic via listeners.

Got a PoC running with DebugWorld. Overriding world.deleteto get the stacktrace at the moment user calls for deletion, which is handled by batchProcessor which isn't exposed in WorldConfiguration (yet). For creation calls a custom entitymanager is supplied.

Hmm, maybe an interface Entity.OnIssuedDelete(-Listener) - doesn't have to go into the Entity class, but could - invoked when World.delete(int) is called? Could do a similar one for Entity.OnIssuedCreate. This type of callbacks are useful for editors too.

    public interface DebugLogStrategy {
        void log(String s);

        void log(MutationStacktrace site);
    }

Any chance of doing it as a SAM type (single abstract method)? With android 8 bytecode now being kosher on android and kotlins lambdas play nice with SAM types - it's pretty neat means of omitting boilerplate. I think a default implementation for void log(MutationStacktrace) would suffice in making it qualify as a SAM type.

or wait... are we running into GWT limitations?

Works pretty great so far, figured out my bug in no time. Coolest thing is the gfycat name gen ;)

Really like the idea and output! I would personally prefer shorter names + maybe custom logic (say, entities name prefix from team/enemy type etc), so better have a gfycat name factory interface ;)

hrrm.. scope.

[DaanVanYperen]

appropriate optional callbacks?

That would be a lot cleaner, originally went for 'no performance impact for production' as a criteria which causes some of these hoops. If you don't see an issue with it, one null check per call isn't /too/ expensive.

SAM seems fine solution. Name from component is a lifecycle thing, at the moment of creation the composition isn't there yet. GWT we can just test.

[DaanVanYperen]

@junkdog See several ways to do this.

• mySystem implement EntityLifecycleListener, scanner gathers impls, simple null check at call sites.
• WorldConfiguration.register(EntityLifecycleListener), allow one, or multiple.

Kinda charmed of the (edit)FIRST one.

Any other callsites you need for editors? ->

/**
 * Listener for events in the entity lifecycle.
 *
 * Use this for debugging or cases where you need to decorate every entity.
 *
 * @author junkdog
 * @author Daan van Yperen
 */
@UnstableApi
public interface EntityLifecycleListener {

    /**
     * Intercept deletion issued.
     *
     * Triggers on {@code deleteFromWorld} invocations, just before the entity
     * is scheduled for deletion. Accessing components is still allowed.
     *
     * Entity deletion is finalized at a later moment in the engine lifecycle.
     *
     * @param entityId
     */
    void onEntityDeleteIssued(int entityId);

    /**
     * Intercept entity post creation.
     *
     * @param entityId Id of created Entity.
     */
    void onEntityCreated(int entityId);

    /**
     * Intercept entity pre get.
     *
     * Triggers when {@code EntityManager.getEntity} is called, just before
     * actually resolving the entity. The entity is not guaranteed to exist.
     *
     * @param entityId id of the entity to get.
     */
    void onEntityGet(int entityId);
}

[junkdog]

That would be a lot cleaner, originally went for 'no performance impact for production' as a criteria which causes some of these hoops.

It should be fine; if it's a hit, we can always ASM it away at a later date (kotlin go very well together with ASM's tree API).

• mySystem implement EntityLifecycleListener, scanner gathers impls, simple null check at call sites.

EntityManager.register(EntityLifecycleListener) - during EntityManager.initialize() auto-registers all systems implementing said interface. Need an unregister(EntityLifecycleListener) too. Too avoid confusing use-cases, allow for any number of listeners.

public interface EntityLifecycleListener

Not sure about onEntityGet() - what's the use-case for it? Otherwise looking good.

[DaanVanYperen]

Not sure about onEntityGet() - what the use-case for it?

This would allow the debug plugin to report the delete-issued stacktrace when illegally accessing a deleted entity.

[DaanVanYperen]

EntityManager.register(EntityLifecycleListener) - during EntityManager.initialize() auto-registers all systems implementing said interface. Need an unregister(EntityLifecycleListener) too. Too avoid confusing use-cases, allow for any number of listeners.

Ah makes sense, the PR is static, I'll make it dynamic.

[junkdog]

onEntityGet() - it feels considerably more heavy than the others and I'm not convinced it captures all the use-cases.

Can we get rid of bit but still find a way forward? How about catching it in InvocationStrategy + maybe a default exception handler (although, I guess that's project-specific)? Still need to find out how to propagate the requested/deleted entity though.

[DaanVanYperen]

NotableNautilus(1) CREATE @ net.mostlyoriginal.game.system.map.TiledMapManager.createLayerEntity(TiledMapManager.java:85)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.load(TiledMapManager.java:73)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.initialize(TiledMapManager.java:62)
NotableNautilus(1) DELETE @ net.mostlyoriginal.game.system.map.TiledMapManager.createLayerEntity(TiledMapManager.java:90)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.load(TiledMapManager.java:73)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.initialize(TiledMapManager.java:62)
*********************
NotableNautilus(1) ERROR_ATTEMPT_TO_DELETE_DELETED_ENTITY @ net.mostlyoriginal.game.system.map.TiledMapManager.createLayerEntity(TiledMapManager.java:91)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.load(TiledMapManager.java:73)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.initialize(TiledMapManager.java:62)
Cause (Already deleted at):
NotableNautilus(1) DELETE @ net.mostlyoriginal.game.system.map.TiledMapManager.createLayerEntity(TiledMapManager.java:90)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.load(TiledMapManager.java:73)
       .. net.mostlyoriginal.game.system.map.TiledMapManager.initialize(TiledMapManager.java:62)
*********************

[junkdog]

Really cool!

[DaanVanYperen]

onEntityGet() - it feels considerably more heavy than the others and I'm not convinced it captures all the use-cases.

Lets just remove that then. It's a little less convenient but users can still use the log to determine the point of deletion with a quick search.

Is there a good usecase for the runtime removable listener feature? Could save a bit of time not making that.

[junkdog]

Lets just remove that then. It's a little less convenient but users can still use the log to determine the point of deletion with a quick search.

NoSuchEntityException?

Is there a good usecase for the runtime removable listener feature? Could save a bit of time not making that.

Not a good enough one - skip it. Everything tends to have their lifetimes aligned with the world's anyway.

[DaanVanYperen]

NoSuchEntityException

Is try/catch free if not triggered? That would require us to WorldManager.getEntity rethrow as NoSuchEntityException?

If it is free, could we just try { } catch ( E e ) { listeners.onGetInvalidEntity(id) }?

[DaanVanYperen]

https://stackoverflow.com/questions/16451777/is-it-expensive-to-use-try-catch-blocks-even-if-an-exception-is-never-thrown

[junkdog]

Yeah, it should probably be benchmark, but I think we'd be fine

[DaanVanYperen]

Have you joined the libgdx discord yet? there's a (text) channel there with a lot of active users, people ask for updated benchmarks frequently.

[DaanVanYperen]

Crap, it is a bag, so when we get what was once a valid id it will probably not throw a ArrayIndexOutOfBoundsException. So we would have to do a null check in addition to the exception handler.

   protected Entity getEntity(int entityId) {
        try {
            final Entity entity = entities.get(entityId);
            if (entity == null)
                throw new EntityNotFoundException("Entity with id " + entityId + " does not exist. Deleted?");
            return entity;
        } catch (ArrayIndexOutOfBoundsException | EntityNotFoundException e) {
            if (entityLifecycleListener != null) {
                // callback on failed get.
                entityLifecycleListener.onEntityNotFoundException(entityId);
            }
            throw e;
        }
    }

[DaanVanYperen]

PoC plugin can be reviewed at https://github.com/DaanVanYperen/artemis-odb-contrib/commit/a3eec84a6c404206165ea241164113dae98631c5

Can report errors OR everything. Just needs a way to supply user chosen (shorter) entity names.

WorldConfigurationBuilder.with(DebugPlugin.thatLogsErrorsIn("net.mostlyoriginal")) WorldConfigurationBuilder.with(DebugPlugin.thatLogsEverythingIn("net.mostlyoriginal"))

Accessor detection isn't great yet, since ComponentMapper doesn't even hit EntityManager if you work with ints, and bums out with a ArrayIndexOutOfBoundsException. perhaps wrapping strategy would work better after all.

[junkdog]

Crap, it is a bag, so when we get what was once a valid id it will probably not throw a ArrayIndexOutOfBoundsException. So we would have to do a null check in addition to the exception handlerr

If we can skip gwt compat for this part of the feature, I can throw together a classloader to record last accessed entity id. Apart from gwt, the caveat is that one needs to activate it before referencing the world class.

[junkdog]

So we would have to do a null check in addition to the exception handler.

It could be argued that it's valid to ask for an entity which doesn't exist. Would likely break someone's code.

[DaanVanYperen]

If we can skip gwt compat for this part of the feature, I can throw together a classloader to record last accessed entity id.

GWT compat is optional IMO, let users spin up desktop if they want to debug. That said, after lurking in the libgdx discord for a bit people already struggle with the build complexity of odb, especially when using the more exotic features. Is there some way to go vanilla without extra hoops?

It could be argued that it's valid to ask for an entity which doesn't exist. Would likely break someone's code.

I agree. The listener shouldn't alter behavior if we can avoid it. I'll revert the getEntity changes after work.

In the end the issues you'd want to catch with DebugPlugin would require some broader hooks anyway. (ComponentMapper for example). Perhaps extending classes (except World) would be a valid trade-off after all. If we jump to proxy classes or runtime class generation might as well just manually subclass and keep things easier to understand. None of that needs to end up in the odb repo, it's mostly debug plugin specific.

To make the PoC production ready plan to write some test cases for the DebugPlugin with all the ways users can shoot themselves in the foot, and try to cover all those. That should give us a better idea what it'll touch.

[DaanVanYperen]

Oh just noticed this on gitter: https://github.com/Namek/artemis-odb-entity-tracker/tree/develop Poke @Namek

At work so haven't investigated where our needs overlap.

[junkdog]

Is there some way to go vanilla without extra hoops?

We can fake it: e.g., DebugPlugin.preInitialize() could set up a classloader to take care of it at startup. Can throw an exception if it's invokedtoo late.

(working on something similar at home)

[DaanVanYperen]

We can fake it: e.g., DebugPlugin.preInitialize() could set up a classloader to take care of it at startup. Can throw an exception if it's invokedtoo late.

I'm interested to see what you are planning with the classloader. supply a (dynamic) proxy?

If it is at all solid we could remove the built-in EntityLifecycleListener and move it to a LifecycleListenerPlugin, hook whatever we need without hurting anyone.

[junkdog]

Yeah, pretty much - inject the callback sites during class loading. I can expand what I mean after I get home from work. It'd be a good stepping stone to kotlinizing the existing ASM code too.

[DaanVanYperen]

I'm down! Good opportunity to improve the plugin API as well. This is getting a decent distraction from polishing my Jam game but at least this can benefit a broader audience. ;)

[junkdog]

(Invited you to cmpapi - api differ working on the bytecode level - which uses a custom ASM-infused classloader for stubbing classes not on the classpath)

[DaanVanYperen]

Usecase criteria updated. I'll take a peek at cmpapi after work!

[DaanVanYperen]

Tests for stuff that is useful to intercept. Anything missing? https://gist.github.com/DaanVanYperen/d7600f3f5b2075ed389a5b3edec6c561

To implement the LifecycleListenerPlugin I need a little more to go on. How would the EntityLifecycleListener impl look like in cmpapi equivalent? We'll need to pass both method parameters and sometimes the result to a listener. Ideally we'd still work with world scoped Listener instance to support multi-world setups.

[junkdog]

I'll take a look shortly!

To implement the LifecycleListenerPlugin I need a little more to go on. How would the EntityLifecycleListener impl look like in cmpapi equivalent?

... hmm, the more I think about it the trickier it gets.

But, I think something like this could work - starting with the dirty:

/* public? */ class StaticDebugEventTrace implements EntityLifecycleListener {
    public static EntityLifecycleListener current;

    public static int entityId;
    public static Type type;
    // ... and remaining fields. stacktrace and cause are null unless relevant

    // convenience - prob with overrides for `stacktrace` and maybe `type`
    // ergo; can be created from any `catch` context 
    DebugEventStacktrace create() { /* impl */ }

    // listener impl over world (consistency + compartmentalizing all the ugly stuff into
    // one place - in the hope that it is easier to refactor if it is later deemed insufficient 
}

The purpose of static mutable debug event is to ensure that a relevant DebugEventStacktrace can be constructed w/o necessarily having to catch the relevant context everywhere (say when using a default exception handler). Alternatively, for a somewhat more flexible solution; provide a static implementation/controller to StaticDebugEventTrace - which could wrap everything in thread locals or something, but it feels like a lot of complexity for little benefit.

The cleaner alternative - skipping the singleton approach - pushes the complexity to the bytecode injection. Multi-threading concerns feel like something we can skip - at least for the time being(?).

DebugLoginPreloader.inject() sets the classloader, and also Thread.setDefaultUncaughtExceptionHandler to alleviate the need for try-catch everywhere. during inject(), checks if World has not been loaded by the classloader - otherwise throw unable-to-debug-world exception.

public class MyGame {
    // step 1: option 1: put it in the static initializer
    static { DebugLoginPreloader.inject(); }

    public static void main(String[] args) {

        // step 1: option 2: put as first statement in main()
        DebugLoginPreloader.inject();

        // step 2: rest of game goes here
        // step 3: ... ?

        // step 4: profit
    }
}

Additionally, DebugPlugin throws during initialize unless World is loaded with the correct classloader; ie, if DebugLoginPreloader.inject() wasn't invoked. There should probably be a convenient opt-out for the this check though, maybe DebugPlugin(boolean enable). DebugPlugin is also responsible for registering the StaticDebugEventTrace with the world (or maybe delegating/driving the actual listener)

The ASM classloader will graft bytecode into ComponentMapper.get, EntityManager and elsewhere - pretty much what the PR contains atm + mapper stuff.

Pretty happy with how it turned out in cmpapi, although it requires some additional thought to it for this scenario. This will, if it works out, allow for code templates/chumks to be written as plain java code and then referenced as transplants:

        val cn = classNode(inputstream) // initial load from jar file/local file

        // EmptyClassTemplate is:    public class EmptyClassTemplate {}
        // class surgery; copy default ctor to `cn` ClassNode
        cn.graft(transplant<EmptyClassTemplate>("<init>", "()V"))

        cn.toBytes() // invoked by Classloader.defineClass

[junkdog]

Have you joined the libgdx discord yet?

no but i should; really liking discord's android app

[DaanVanYperen]

Looking at this i'm not clear on the benefit of bytecode manipulation over just subclassing.

Besides it being slightly faster wouldn't it be fairly fragile and hard to maintain compared to just subclassing ComponentMapper, EntityManager and BatchProcessor? It's not too clean either way sure, but I personally I wouldn't care much if my debugger runs at 25% of normal speed.

[junkdog]

The bytecode stuff should be relatively easy to implement (famous words) and unit test; classloader may have a gotcha or two.

Benefit vs investment is probably not calculated properly, but subclassing is problematic w.r.t. ComponentIdentityResolver and BatchChangeProcessor being final. BatchChangeProcessor is preferably kept final to allow for internal refactorings (it's effectively internal). That aside, pretty quick/pragmatic.

Doing the transformations at runtime, one could enable intercepting all .get(entity), plus whatever other options one wants.

[DaanVanYperen]

Allowing max 1 world instrumented seems fine to me, as any issues are world local anyway and I haven't even seen a multiworld impl in the wild.

bytecode vs subclassing&non final internals.

It's up to you. Subclassing I can solo. Bytecode you'll have to put some time towards.

[junkdog]

I can do some prototyping/experimentation during the weekend - get a feel for what's required, possibly commit something useful.

[DaanVanYperen]

Ok. I'll see towards some more clarity on the needed hooks for my use case. Should be able to produce a more concrete listener interface. Probably best to store the static reference for the singleton listener separate from the interface and keep it lean and mean.

AspectJ support runtime stuff? Haven't looked at that in forever.

[junkdog]

Never touched AspectJ - wasn't it hindered by a software patent?

So, for defining the bytecode-transplanting templates - this is how I envision they'd look/function:

// package is unimportant / not our concern
package mygame.artemis.hello;

import com.artemis.ComponentMapper;

// required annotation: select target class 
@Graft.Target(ComponentMapper.class)
// name can be anything, but generic signature must match
public class ComponentMapperTransplant<A> { 

    @Graft.Prepend // one could possibly also Graft.Append, Graft.Replace, Graft.Skip
    public A get(int entityId) throws ArrayIndexOutOfBoundsException {
        log(entityId, Type.MAPPER_GET, false);
        return get(entityId); // N.B. seemingly recursive call
    }

    // method signature must match target method
    @Graft.Prepend
    public A create(int entityId) {
        log(entityId, Type.MAPPER_CREATE, true); 
        return create(entityId);
    }

    // non-annotated methods copied to target class as-is (validation error if already exists)
    private static void log(int entityId, Type type, boolean generateStackTrace) {
        // this assumes that StaticDebugEventTrace is accessible
        // when writing these templates
        StaticDebugEventTrace.entityId = entityId;
        StaticDebugEventTrace.type = type;

        // maybe stacktrace or other loggable stuff
    }
}

• the "recursive" call acts as a known marker in the ASM code (easy to intercept), but can conceptually be thought of as continuing with the original method
• Transplant classes are annotated with Graft.*
• Only @Graft.Target is strictly required
• @Graft.Skip is useful to make compilation pass; e.g. @Graft.Skip World world;
• fields can also be transplanted
• can be expanded to more use-cases/classes without requiring changes to the underlying instrumentation code
• In theory, this approach mitigates the need for the singleton altogether

Feedback welcome. It's a bit quirky perhaps, but it doesn't have a lot of moving parts from the user POV.

[junkdog]

This approach would also result in getting rid of almost all the old ASM code - most of the existing stuff can be expressed with *Transplants.

[DaanVanYperen]

The grafting API looks good. I suspect I'm not getting the limitations or how this circumvents singleton (isn't that what StaticDebugEventTrace would be?)

I'm splitting the debugger into a listener plugin and a debugger plugin, but that doesn't change the grafting API itself I guess. How flexible is this? Would this be legal?

@Graft.Target(ComponentMapper.class)
public class ComponentMapperTransplant<A> { 

    @Graft.Prepend
    public A create(int entityId) {
        LifecyclePlugin.dispatcher.onComponentPreCreate(world, entityId);
        A result = create(entityId);
        LifecyclePlugin.dispatcher.onComponentPostCreate(world, entityId);
        return result;
    }
}

[DaanVanYperen]

Just as a final challenge to the pro/cons, this implements something to circumvent our own final classes which we control. And is admittedly cool.

[junkdog]

I suspect I'm not getting the limitations or how this circumvents singleton (isn't that what StaticDebugEventTrace would be?)

Well, you could inject - either relying on world.inject or manually setting up the listeners in initialize() or the constructor. The static approach - or your dispatcher - is easier though.

@Graft.Mock is perhaps a better name than @Graft.Skip;

@Graft.Target(EntityEdit.class)
public class EntityEditTransplant
       // all calls to `cm` will refer to EntityEdit.cm after  transplant.
       // signature must match, but anything goes (i think)
    @Graft.Mock private ComponentManager cm;

    @Graft.Prepend    
    public EntityEdit remove(ComponentType type) {
        System.out.prtintln(cm.toString()); // accessible thanks to mock
        return remove(type);
    }

How flexible is this? Would this be legal? ` perfectly legal code

Yes! When a method is Graft.Prepend:ed, the original method is renamed by appending '$actual` to the name. So, in the remove case, "remove$actual(...)". When grafting a method, all "recursive calls" are translated so that they instead point to the original, "$actual" method. So, you're free to call the $original or opt out completely.

this implements something to circumvent our own final classes which we control. And is admittedly cool.

:yak: :smile:

[DaanVanYperen]

Pfew this is becoming quite a scroll! I wonder if tickets cap at 100 comments.

[DaanVanYperen]

PoC done! :) Just need to pivot subclasses to @Grafts.

[DaanVanYperen]

Debug plugin is finding issues in Jam library i've been using for years.

(delete component after entity.removeFromWorld).

*********************
MediumblueLeafcutterant(238) ERROR_ATTEMPT_TO_ACCESS_DELETED_ENTITY @ net.mostlyoriginal.api.plugin.extendedcomponentmapper.M.remove(M.java:146)
       .. net.mostlyoriginal.api.plugin.extendedcomponentmapper.M.remove(M.java:156)
       .. net.mostlyoriginal.api.system.SchedulerSystem.process(SchedulerSystem.java:31)
Caused by ENTITY_DELETE at:
MediumblueLeafcutterant(238) ENTITY_DELETE @ net.mostlyoriginal.api.operation.basic.DeleteFromWorldOperation.process(DeleteFromWorldOperation.java:19)
       .. net.mostlyoriginal.api.operation.common.BasicOperation.process(BasicOperation.java:16)
       .. net.mostlyoriginal.api.operation.flow.SequenceOperation.nextOperation(SequenceOperation.java:37)
       .. net.mostlyoriginal.api.operation.flow.SequenceOperation.process(SequenceOperation.java:25)
       .. net.mostlyoriginal.api.operation.flow.ParallelOperation.process(ParallelOperation.java:22)
       .. net.mostlyoriginal.api.system.SchedulerSystem.process(SchedulerSystem.java:29)
*********************```

[DaanVanYperen]

Mostly done. Only grafting & fix debug plugin reporting listeners interacting with deleted entities as errors.

[junkdog]

I should have posted https://github.com/junkdog/artemis-odb/issues/576#issuecomment-503305890 here instead.

I'll begin replacing the existing ASM code in artemis in the coming days (away over midsummer though). It's pretty sparse on tests, so might be a bumpy ride, but my gut feeling is pretty optimistic.

[DaanVanYperen]

That's one fine looking library! What we could do is if you line the ASM rewrite for 2.4.0, in the mean time I'll rewrite https://github.com/junkdog/artemis-odb/pull/575 with graft so we can get a release out for the peeps before summer vacation. Think there's a chunk of stuff ready for release https://github.com/junkdog/artemis-odb/blob/develop/CHANGELOG.md

[junkdog]

Thank you! Very happy with how this turned out. Still some rough edges of course (better validation, both agent and maven plugin could use some extra configuration etc), but code is pretty clean/easy to extend.

Yeah, wise to delay the ASM rewrite for 2.4.0. We're not adding any functionality to that layer anyways, and like you said - the changelog is growing.

[junkdog]

WIP on integration tests-as-recipes for the agent: figured they could be recycled on the wiki.

https://github.com/junkdog/graftt/tree/master/agent/src/it/agent-no-params/src