Add Loop and Conditional extensions to the SHOP domain language #142
Conversation
|
@ukuter or Also, please put a description of what this branch aims to achieve into the PR description box. The above two things will live on in the SHOP3 repo history and need to be informative to help any future maintainers. |
| (cond | ||
| ((primitivep (get-task-name task)) | ||
| (setf (mode state) 'expand-primitive-task)) | ||
| ((eql (get-task-name task) :loop) |
There was a problem hiding this comment.
Is there a reason to use keywords here instead of just using the normal symbols?
I know this is a nit, but SHOP syntax is already full of inconsistencies about use of keywords versus normal symbols, and I would like to move towards consistency by killing all use of keywords inside method definitions, and reserve them only for :op, :method, etc.
|
|
||
| (defparameter *output-form* nil) | ||
|
|
||
| (defmethod print-node ((plan-tree-node top-node)) |
There was a problem hiding this comment.
Please add a defgeneric with docstring to explain what this is for and how to invoke it. Do we call print-node directly, or is there an outer function to invoke?
When we get the docstrings, we should reference them in the manual (I can explain how this is done).
| )) | ||
| ) | ||
|
|
||
| (defun make-conditional-state-expand (&rest arglist |
There was a problem hiding this comment.
(declaim (ftype (function (&rest arglist &key :top-tasks :tasks :unifier) (values conditional-state-expand &optional) make-conditional-state-expand))
maybe?
There was a problem hiding this comment.
I think that this is syntactically wrong: make-conditional-state-expand (function name) is an argument of ftype and not part of function (the type description).
shop3/looping-tasks/exts-common.lisp
Outdated
| (:documentation "Mixin domain class for HTNs with LOOP tasks in them.")) | ||
|
|
||
|
|
||
| (defmethod seek-plans-task ((domain looping-domain) task1 state tasks |
There was a problem hiding this comment.
Could this be replaced by a simpler method that checks for (looping-p (get-task-name task1)) and then calls seek-plans-loop or if that test fails, simply call-next-method?
By squashing all of the seek-plans-task code for other classes and mixins, this potentially breaks present and future code for other SHOP extensions.
shop3/looping-tasks/exts-common.lisp
Outdated
| backtrack-stack))))) | ||
| ess-search-state) | ||
|
|
||
| (defun new-symbol (sym) |
There was a problem hiding this comment.
Never use intern without specifying the optional package argument, otherwise you just get whatever *package* is bound to when new-symbol is called.
Also gentemp = gensym followed by intern, so just use gentemp
There was a problem hiding this comment.
Sorry to bother you again, but what about this:
(defun new-symbol (sym &optional (pkg-desig :shop)) ...)
I just suggest this because so far we are guessing what Ugur wanted originally. Not a big deal.
matthewdehaven
force-pushed
the
lce-rebase
branch
from
965900f
to
f3adfb8
Compare
|
Awesome progress in just 1 day, @matthewdehaven ! |
to enable disabling debug prints.
Fix syntax error and unused `rationale` argument.
Undefined variable (misspelling) and variable that is written but not read.
Setting up to print trace information caused us inadvertently to rebind `state`, breaking a call to `stack-backtrack`.
This condition is removed in the original `loop-conditional-exts` branch and replaced with just the else condition. This change matches the behavior on that branch.
And a package name is provided
rpgoldman
force-pushed
the
lce-rebase
branch
from
ecab1b9
to
ae9da93
Compare
Only handle the loop tasks and defer primitive and non-primitive handling to `call-next-method`. This avoids duplicate code which could break other extensions.
No description provided.