FEOMedia / artemis-odb-orion

Operations mini-DSL specialized to deal with artemis-odb entities
Apache License 2.0
21 stars 5 forks source link

Feature request: call function end when operation finishes #11

Closed sonirico closed 8 years ago

sonirico commented 8 years ago

Sometimes my operations manipulates components, like MotionComponent. It would be very useful that at least TemportalExecutor provided an end() method -like begin(Operation, OperationTree)- for cleanup purposes or removing no longer required components.

I have more questions, such as the purpose of the Friend class and how to dynamically add new operations inside operations if possible. Is there a gitter for orion?

junkdog commented 8 years ago

It would be very useful that at least TemportalExecutor provided an end() method

:+1:

I have more questions, such as the purpose of the Friend class

not allowing internal methods to be called from the public API; trick from top-voted answer to http://stackoverflow.com/questions/182278/is-there-a-way-to-simulate-the-c-friend-concept-in-java

how to dynamically add new operations inside operations if possible

hmm, why do you want to do this? it's - i think - possible, but you probably want to sub-class ParentingOperation and make sure that the isCompleted check is dynamic + operations added mid-operation work as expected (not sure what expected behavior would be - i think you need to decide the constraints yourself). dynamic operation trees are supported by repeat() and forever(), as they restore the operation back to ithe original state (from a copy during initial init).

Is there a gitter for orion?

I initially thought we'd use artemis-odb's, but it might be a good idea to do one for orion too - just to keep discussion focused on orion. it can be a little hard to excavate from artemis' archives otherwise.

sonirico commented 8 years ago

Well, I should study in deep more each built-in operation, but at first glance I thought that OperationTree was the interface to add new operations on the way, useful for some IA cases. Anyway, that does not bother me so much currently. Sequencing operations is flexible enough. I am forking this repo and I will try to add that end method. It would be fancy improving things as return for using then for my stuff xD

sonirico commented 8 years ago

I tried, but somehow the method Executor.end(Operation, OperationTree) never gets called. https://github.com/sonirico/artemis-odb-orion/commit/9366457452b9d0df82276d439e01923dab57db11

junkdog commented 8 years ago

I tried, but somehow the method Executor.end(Operation, OperationTree) never gets called.

It's probably because the outer operation completes before end has a chance of being invoked.

I think the end() invocation needs to come around the method return point, in an additional if (operation.isComplete()) check after the act - otherwise you can end up in situations where end is called, but begin isn't.

Well, I should study in deep more each built-in operation, but at first glance I thought that OperationTree was the interface to add new operations on the way

OperationTree is the wrapper around operations; each node containing its executor reference + operation. You can't construct it yourself; but you can interact with it via executors. Take a look at RepeatOperation, it does:

/** resetting by replacing completed operation */
private OperationTree replaceChild(OperationTree node) {
    RepeatOperation op = (RepeatOperation) node.operation;

    OperationTree oldChild = node.children().pop();

    // registered operations are "consumed", hence the copy
    Operation copy = operations.copy(op.repeated);
    node.add(copy.toNode());

    // only initialize the replaced node + any children
    node.initialize(operations, op.entityId);

    // reclaiming objects
    oldChild.clear();
    return node.children().first();
}
sonirico commented 8 years ago

Ok. I called isComplete again after act and all tests are passing. Before the PR: Is there a way to show here that all tests are passing besides a seedy screenshot? xD

junkdog commented 8 years ago

Nah, i'll do it old way - pull-test-merge. Need to configure travis with this repo, but may take some time.

sonirico commented 8 years ago

I have been using this tweak for a little time now and it works pretty neatly. Maybe is it time to close this issue?

junkdog commented 8 years ago

:+1: