Skip to content
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

Working version of RLBotJavascriptExample #3

Open
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

SuperVK
Copy link
Collaborator

@SuperVK SuperVK commented Aug 24, 2020

The first working and tournament ready version of RLBotJavascriptExample

@tarehart
Copy link

This worked nicely in the tournament, easy to set up!

Would it be appropriate to add .yarn/ to gitignore? Feels weird to have a whole bunch of cached packages in the example repo. Maybe some of the other stuff too.

@SuperVK
Copy link
Collaborator Author

SuperVK commented Aug 25, 2020

The thought behind this is that you dont need to run npm install and possibly have to install other compiling tools. Checking it in the repo is a stragety suggested by yarn and I also did it with the example to make it feel as complete as possible (this example isn't fully runnable from directly cloning because I didn't check in the .yarn/unplugged folder because that contained unzipped node_modules.

You could let the developers install yarn themself, upgrade to yarn 2 and then let them add it to their own repositories (or zip the .yarn folder with the other files when sending to a tournament runner). But because the packages are zipped, it's not that many files, but I agree it feels weird to have the .yarn and the .pnp.js folders/files in here.

(Eldest did propose a solution where developers dont need to use yarn, but we need to make some changes to the gui for that)

@tarehart
Copy link

Ah ok, that makes sense

Copy link

@tarehart tarehart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried the steps in the README using powershell and it got upset:

PS C:\Users\tareh\code\RLBotJavascriptExample> yarn
yarn : File C:\Users\tareh\AppData\Roaming\npm\yarn.ps1 cannot be loaded. The file
C:\Users\tareh\AppData\Roaming\npm\yarn.ps1 is not digitally signed. You cannot run this script on the current system.
For more information about running scripts and setting execution policy, see about_Execution_Policies at
https:/go.microsoft.com/fwlink/?LinkID=135170.
At line:1 char:1
+ yarn
+ ~~~~
    + CategoryInfo          : SecurityError: (:) [], PSSecurityException
    + FullyQualifiedErrorId : UnauthorizedAccess

Worked fine in cmd, great stuff! Sorry for taking so long.

Comment on lines +86 to +95
return controller; //yes this returns before the gravity get canceled, so move the return if you want to have 0 gravity

//cancel gravity
//PROBABLY WANNA REMOVE THIS
let cars = []
for(let car of gameTickPacket.players) {
cars.push(new CarState(new Physics(null, null, new Vector3(null, null, car.physics.velocity.z+(650/60))))) //assumes you are running 60fps
}
let ball = new BallState(new Physics(null, null, new Vector3(null, null, gameTickPacket.ball.physics.velocity.z+(650/60)))) //assumes you are running 60fps
this.setGameState(new GameState(ball, cars))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth removing this or maybe turning it into a commented-out function if you want people to be able to test state setting easily. Likewise for the js version.

Is there a good reason to have a .js version and a .ts version of the example bot? Is this just to make it obvious that people have a choice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to show off all the api functions in one file, but I think I already show how to set a new state previously. So I'm probably going to remove this to avoid confusion. (It's messy with the 60 fps either way)

And for the ts and js file, it's mainly to make it clear you can use typescript aswell, I don't think it's worth to make a seperate RLBotTypescriptExample, so I thought this would be the simplest.

Comment on lines +22 to +27
#self.runner = None
#try:
# self.runner = subprocess.Popen([os.path.realpath(os.path.join(os.getcwd(), os.path.dirname(__file__), 'auto-run.bat'))])
#except Exception as e:
# self.runner = None
# self.logger.error(f"A JavaScript bot with the name of {self.name} will need to be started manually. Error when running 'auto-run.bat': {str(e)}.")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of commented-out code in this file, would be good to remove if possible.

Also, have you considered whether you want to put this in the RLBot framework so that we can push fixes to people? It actually seems very similar to https://github.com/RLBot/RLBot/blob/master/src/main/python/rlbot/agents/executable_with_socket_agent.py, is there a meaningful difference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants