JuliaReinforcementLearning / GridWorlds.jl

Help! I'm lost in the flatland!
MIT License
46 stars 9 forks source link

Added pickup and drop actions #38

Closed landrumb closed 3 years ago

landrumb commented 3 years ago

pickup and drop functions were added in response to issue #36, an inv attribute was added to Agent, a new abstract class Item <: AbstractObject was added to indicate objects that could be picked up, and Key and Gem were changed to inherit from it.

Outside of objects.jl, doorkey.jl was changed to use the pickup function and inv attribute of Agent, but its functionality is the same as before.

As is, when an agent picks up or drops their inventory is up to the environment to decide. In doorkey.jl I left the existing behavior where walking over the key picks it up, but in an environment such as ObstructedMaze (#34) where items have to be dropped, there needs to be an interface besides walking over the object one wants to pick up. If the default Null object was replaced with Empty <: Item, such that empty spaces could be picked up, it would allow picking up and dropping items by replacing the Empty object in the inventory with something else and vice versa, but this would not work with the automatic pickup used in doorkey.jl, because the key would be automatically dropped on every forward movement.

codecov-io commented 3 years ago

Codecov Report

Merging #38 into master will decrease coverage by 3.22%. The diff coverage is 7.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
- Coverage   36.06%   32.83%   -3.23%     
==========================================
  Files          13       13              
  Lines         305      335      +30     
==========================================
  Hits          110      110              
- Misses        195      225      +30     
Impacted Files Coverage Δ
src/envs/doorkey.jl 0.00% <0.00%> (ø)
src/objects.jl 8.47% <8.57%> (-8.77%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 495419c...0fc0d26. Read the comment docs.

landrumb commented 3 years ago

I am not confident that my implementation of traits is correct. I would appreciate if someone could look over my last commit and confirm that it's done correctly.

sriyash421 commented 3 years ago

@findmyway @landrumb Sorry, I might be repeating something that might have been discussed earlier but I wanted to ask something. Why can't we use abstract objects to define traits such as Pickable = Union{Key, Gem} and check if Obj <: Pickable ??

landrumb commented 3 years ago

@sriyash421 If I understand your question, what you're describing is essentially the same as what we do with traits, but the advantage of using traits is that you can more fluidly build it into the dispatch for functions that rely on the type annotation. However, I could be wrong as I've only known about traits for about 2 days. @findmyway can confirm or deny if that's the reason we're using traits, as he's more knowledgeable on the topic.

findmyway commented 3 years ago

Yes @landrumb is mostly right here. @sriyash421 Suppose one wants to create a new environment and needs to create a new object. And this object happens to be a pickable object. If we adopt the union method, then one has no way but to change the source code of our package. But with the trait way, one can simply extend the isitem method.