-
Notifications
You must be signed in to change notification settings - Fork 0
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
Pacman demo #63
Pacman demo #63
Conversation
> Split headers from source files for a clean install
This uses the submodule version of EnTT instead of the installed version as a workaround since the installed version gave me compile errors.
Update positions in separate function and move to cpp
…nit test Remove unnecessary class member
Implement a wrapper class for the pacman game that handles all the SDL stuff, the game loop etc. This simplifies the main.cpp by quite a bit and allows to see whats actually important.
Co-authored-by: Piotr Spieker <[email protected]> - Remove MRT leftovers - Remove unnecesseary doxygen string - Do not track isActive in runAwayFromGhostBehavior - Undo feedback commit: Position operator+ cannot return a reference - Rename PacmanArbitrator to PacmanAgent - Rename Direction::NONE to Direction::LAST - Consider the tunnel when computen the distance in A* - Replace global variable possibleMoves by static function - Fix tests where aStar was not properly initialized - Remove mounting of /dev/dri - Fix some includes
…ults - Function returns both the distance and the position to remove unecessary recompute - Cache closestGhost computation using util_caching
Optionally render planned path
Improve docker images (versioning, cleanup)
Alright, I've updated the Readme a bit. Regarding the feature list, this could be an alternative formatting (a bit cleaner, but longer): Details
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good, thanks for the readme update!
I added a couple of points but feel free to merge without them.
demo/docker-compose.yaml
Outdated
args: | ||
- VERSION=$VERSION | ||
target: tutorial | ||
profiles: ["tutorial"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, didn't know this one yet. But with the demo service not having a profile, wouldn't that just launch both services if you add --profile tutorial
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so I'm bit of misusing the profile thing in order to get the demo ad in the Readme as catchy as possible 🙈
In the proposed setup, I'd suggest not to use --profile
at all.
Instead,
docker compose up
will run the demo service (only)docker compose run --rm --service-ports tutorial
will be used for the tutorial (only)
In order to avoid docker compose up --profile tutorial
from launching the demo service as well, w'd have to assign a demo
profile to the latter.
But, docker compose up
wouldn't run anything then and we'd have to advertise the demo with docker compose up demo
Not a big deal, rather opinionated perfectionism 🤓
What do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I get it now. I like the simplified version for launching the demo but maybe we should add an explanatory comment somewhere? I guess it is a bit of a hack and without knowing the context it is not very obvious why the profiles:
setting is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, see 729b1fe for the docs comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thanks 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, I just realized that this affects building the images as well:
arbitration_graphs/demo ❯ docker compose build
now only builds the demo service, not the tutorial – that's a bit confusing 🤦
On the other hand, I guess the end users won't run into this (only relevant when changing the service definition) 🤷
Also we can promote using the --build
flag on docker compose run
Co-authored-by: ll-nick <[email protected]>
See also a45bea0 for a tiny rephrasing based on ChatGPT feedback |
Oh, and one more thing: The |
Right, deleted with e947286 |
Merged! 🚀 |
Finally, we're very close to merge the long-awaited demo.