Kinds-of-Intelligence-CFI / animal-ai-unity

Animal-AI Unity
https://github.com/Kinds-of-Intelligence-CFI/animal-ai
Apache License 2.0
2 stars 2 forks source link

Feature hollow object #36

Closed alhasacademy96 closed 5 months ago

alhasacademy96 commented 5 months ago

Proposed change(s)

Firstly, added a new game object with functionality (foundational). SpawnerHollowBox lets the user spawn a reward on top of a hollow object, in this case a box with an opening. The box behaves as a regular game object adhering to common constraints and functionality, such as setting random spawning and colors.

Secondly, a simpler, none functional HollowBox is added per Lucy's request. This object does not spawn rewards.

Functionality:

  1. Spawns any reward, such as GoodGoal or DecayGoal.
  2. Setting a reward to spawn on top of the hollow object as a coherent property (you dont have to specify a new !Item in the yaml) - all in one contained.
  3. Optional - adding a delay to spawning the reward (left false by default).

Currently, the object can only spawn one reward at a time or episode.

Finally, a new UI element is introduced -AAI Build Version Reference UI. This is a simple new UI element that is manually replaced every time a new build is released, for the convenience of the user (quick reference on the build they are using). EDIT: Added a new lightweight script to automate and streamline this UI called BuildVersionControl.

Useful links (Github issues, ML-Agents forum threads etc.)

Types of change(s)

Checklist

Other comments

Example YAML file:

!ArenaConfig
# Testing the Hollow-Obj with foundational parameters.
randomizeArenas: false 
showNotification: false 
canResetEpisode: true 
canChangePerspective: true
arenas:
  0: !Arena
    passMark: 0
    timeLimit: 250
    items:
    - !Item
      name: Agent
      positions:
      - !Vector3 {x: 20, y: 0, z: 20}
      rotations: [0]
    - !Item
      name: SpawnerHollowBox
      positions:
      - !Vector3 {x: 20, y: 0, z: 25}
      rotations: [0]

Screenshots (if any)

Screenshot 2024-04-24 185218

benaslater commented 5 months ago

I came away from the discussion on Tuesday feeling that SpawnerHollowBox didn't have the versatility users would need from hollow boxes (e.g. what if I want to do a test where one of the boxes is empty, or contains a bad reward or a pushable block?). Do we definitely want to include it?

Each feature we include is more maintenance overhead for us in the future, and more chance users get confused between different features (as a new user, how should I make the choice between SpawnerHollowBox and HollowBox? What happens if I write a bunch of tests with SpawnerHollowBox, then I realise I want an empty box and so now I've got to change to using HollowBox but I've still got a load using SpawnerHollowBox hanging around etc). I think I would advocate for adding HollowBox now and keeping the code for SpawnerHollowBox somewhere safe for if when we can make a case that automating this bit will be a really big benefit for users.

alhasacademy96 commented 5 months ago

Yes I thought about that too but I just wanted to get Kozzy in to this as well so I pushed to keep two functional game objects.

The difference in the two objects is clear in my mind and properly documented to allow the users to choose which suits them better.

I didn’t understand your arguments regarding testing. Could you elaborate? On 25 Apr 2024 at 12:22 +0100, benaslater @.***>, wrote:

I came away from the discussion on Tuesday feeling that SpawnerHollowBox didn't have the versatility users would need from hollow boxes (e.g. what if I want to do a test where one of the boxes is empty, or contains a bad reward or a pushable block?). Do we definitely want to include it? Each feature we include is more maintenance overhead for us in the future, and more chance users get confused between different features (as a new user, how should I make the choice between SpawnerHollowBox and HollowBox? What happens if I write a bunch of tests with SpawnerHollowBox, then I realise I want an empty box and so now I've got to change to using HollowBox but I've still got a load using SpawnerHollowBox hanging around etc). I think I would advocate for adding HollowBox now and keeping the code for SpawnerHollowBox somewhere safe for if when we can make a case that automating this bit will be a really big benefit for users. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

benaslater commented 5 months ago

PR nit: I would put AAI Build Version Reference UI in a separate PR PR nit: I would include links to the documentation update in this PR (I think I missed this in my last PR) PR nit: As we don't have automated testing I would include explicit explanation of the testing you've done for this in the PR description (e.g. for this PR, I can see a new test config with SpawnerHollowBox, so I can infer you have tested that, and I would guess you've also tried HollowBox but that isn't clear from the PR description. What about edge cases, tests confirm user errors give the correct response etc?)

alhasacademy96 commented 5 months ago

Yes I agree links to docs updated would be useful. I also forgot to add the new test config reference on here (will be added later today!)

I don't think a separate pr for the new UI is needed as it's a simple addition (there's no code) and is explained in this PR sufficiently. It's not relevant to any other aspect of AAI (I.e teaming agents). Any suggestions and comments on the UI is suitable here.

As I've mentioned before, I have test configs that I setup for each and every new object I add or modify - these are named and placed in the test configs folder in unity accordingly. Yes thank you for bringing small details on to the table, say, what's been tested and how- these additions will help to include them.

We will discuss these aspects tomorrow in detail!

benaslater commented 5 months ago

I didn’t understand your arguments regarding testing. Could you elaborate?

Assuming it's this bit:

What happens if I write a bunch of tests with SpawnerHollowBox, then I realise I want an empty box and so now I've got to change to using HollowBox but I've still got a load using SpawnerHollowBox hanging around etc

tl:dr I'm suggesting that in this case I don't think more features is necessarily better for users (sorry for the essay):

Say I'm a researcher, and I want to do some experiments where you interact with a box somehow and a reward falls out, I might find SpawnerHollowBox and (without doing much forward planning) choose to write my experiments using it.

A big benefit of a box is that it can obscure the things inside it, so maybe after my initial experiments have gone well I'd like to change it so that sometimes the box has a good goal in it, and sometimes it has a bad goal (or nothing). Now I come back to the documentation for SpawnerHollowBox and I realise that it doesn't have that feature. Luckily we also have HollowBox, so now update my generation scripts to generate that instead of the spawner ones. Then probably for consistency / laziness since I don't want to keep switching between box types I always use HollowBox from then on. Now I've got this debt of the old configs on SpawnerHollowBox which I'll either have to update of live with some issues:

My feeling from this story is that I would've been better off using HollowBox from the start, especially given it's not much more complicated to just put a reward above the box in the config myself to replicate the spawner

alhasacademy96 commented 5 months ago

Thank you for the explanation from a researchers point of view - exactly what I need. I will learn to think like that in addition to all other perspectives.

Since yourself and Lucy have highlighted that a simple addition is a much better use case, I will make the necessary changes to this PR today.

You can discard the pr for now until I let you know it's ready