-
Notifications
You must be signed in to change notification settings - Fork 110
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
[FEATURE] Debugging invalid references. #570
Comments
Do we really need to extend the world? A custom 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 |
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 Got a PoC running with 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 ;)
|
Going to bed, already late, (belated hi!), so forgive the brevity - but this was cool, so I had to reply.
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.
Hmm, maybe an interface Entity.OnIssuedDelete(-Listener) - doesn't have to go into the Entity class, but could - invoked when
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 or wait... are we running into GWT limitations?
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. |
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. |
@junkdog See several ways to do this.
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);
} |
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).
Not sure about |
Not sure about This would allow the debug plugin to report the delete-issued stacktrace when illegally accessing a deleted entity. |
Ah makes sense, the PR is static, I'll make it dynamic. |
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 |
|
Really cool! |
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. |
Not a good enough one - skip it. Everything tends to have their lifetimes aligned with the world's anyway. |
Is try/catch free if not triggered? That would require us to If it is free, could we just |
Yeah, it should probably be benchmark, but I think we'd be fine |
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. |
Crap, it is a bag, so when we 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;
}
} |
PoC plugin can be reviewed at DaanVanYperen/artemis-odb-contrib@a3eec84 Can report errors OR everything. Just needs a way to supply user chosen (shorter) entity names.
Accessor detection isn't great yet, since ComponentMapper doesn't even hit EntityManager if you work with ints, and bums out with a |
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. |
It could be argued that it's valid to ask for an entity which doesn't exist. Would likely break someone's code. |
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?
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. ( 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. |
Oh just noticed this on gitter: https://github.com/Namek/artemis-odb-entity-tracker/tree/develop At work so haven't investigated where our needs overlap. |
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. |
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
}
}
Feedback welcome. It's a bit quirky perhaps, but it doesn't have a lot of moving parts from the user POV. |
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. |
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;
}
} |
Just as a final challenge to the pro/cons, this implements something to circumvent our own |
Well, you could inject - either relying on
@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);
}
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.
:yak: 😄 |
Pfew this is becoming quite a scroll! I wonder if tickets cap at 100 comments. |
Debug plugin is finding issues in Jam library i've been using for years. (delete component after
|
…ory to support editor and debugger plugins. - New extension point for debuggers and editors. (`InternalFactory`) - New lifecycle phase listener for debuggers and editors (`ArtemisPhaseListener`).
…ssues. ** Builds against https://github.com/junkdog/artemis-odb/tree/feature-570-lifecycle-poc - *New plugin `contrib-plugin-lifecycle-listener`* - Allows listening to odb component, entity and other lifecycle events. - *New plugin `contrib-plugin-debug`* - Allows debugging those nasty 'entity id' issues. - Can log entity lifecycle events like create, delete. - Can also log errors (accessing/deleting already deleted entities) and report a stacktrace to the cause. - Usage (pick one you like): `WorldConfigurationBuilder.with(DebugPlugin.thatLogsErrorsIn("net.mostlyoriginal"))` `WorldConfigurationBuilder.with(DebugPlugin.thatLogsEverythingIn("net.mostlyoriginal"))` `WorldConfigurationBuilder.with(new DebugPlugin(new MyDebugLogStrategy()));`
Mostly done. Only grafting & fix debug plugin reporting listeners interacting with deleted entities as errors. |
…emoval. - Debugger treats creating components on deleted entities as a bad practice and reports it as an error. - Debugger treats all other component interactions legal on @DelayedEntityRemoval until deletion is finalized.
…nk log output directly to source.
I should have posted #576 (comment) 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. |
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 #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 |
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. |
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 |
deployed latest graftt -SNAPSHOT + begun writing a more in-depth guide on the wiki |
Hey, I'd love to try this to debug some issues I have. |
@intrigus Daan needs to merge/review DaanVanYperen/artemis-odb-contrib#130 first - it should work if you |
@intrigus |
I got it compiling with Java 8.
|
Thanks. Pushed an untested fix, removing usage of SharedSecrets and the unused corba import. |
I'm happy how graftt turned out on contrib. Thanks for that @junkdog! Travis build is passing. This ticket is pretty much done, just waiting for some snapshots to release and then we have a go! |
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)
Code example
To log all calls coming from
net.mostlyoriginal
package.log output (Proof of concept)
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
.The text was updated successfully, but these errors were encountered: