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

Tim/frame rate #100

Merged
merged 21 commits into from
Mar 11, 2021
Merged

Tim/frame rate #100

merged 21 commits into from
Mar 11, 2021

Conversation

tim-macphail
Copy link
Contributor

@tim-macphail tim-macphail commented Mar 2, 2021

Please someone have a look at this, not sure if I've done everything right. I made the already existing FRAME_RATE in core.py accessible to the user by making it an ignite_global. I also added a sentence to the documentation mentioning the frame rate. Closes #81

@AlphaRLee
Copy link
Member

Somehow our main branch is ahead of dev w.r.t. things like the mouse_is_pressed docs. Let's keep an eye open when we merge dev into main later so as to not accidentally wipe out main.

Comment on lines 441 to 450
"source": [
"%%ignite\n",
"\n",
"def setup():\n",
" size(200, 100)\n",
" background(\"light blue\")\n",
" FRAME_RATE = 1\n",
" print(\"frame rate:\", FRAME_RATE)\n",
" # this prints 1, not what I want\n"
]
Copy link
Member

Choose a reason for hiding this comment

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

Please clean up tests like these. In this case, just outright delete this and make sure your other comments are ready-to-go

Comment on lines 652 to 661
"source": [
"%%ignite\n",
"\n",
"def setup():\n",
" size(200, 100)\n",
" background(\"light blue\")\n",
" print(\"frame rate:\", FRAME_RATE)\n",
" FRAME_RATE = 10\n",
" # this gives an error, FRAME_RATE is referenced before assignment"
]
Copy link
Member

Choose a reason for hiding this comment

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

Same thing, delete this test

Copy link
Member

@AlphaRLee AlphaRLee left a comment

Choose a reason for hiding this comment

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

We're missing docs on how to access the FRAME_RATE variable and what it does, could you please add that? Please also add a note that you should treat FRAME_RATE as read-only and don't try re-assigning it.

Added a description of FRAME_RATE to utilities.md
@tim-macphail tim-macphail merged commit 12f9fe7 into dev Mar 11, 2021
@AlphaRLee AlphaRLee deleted the tim/frame_rate branch March 12, 2021 00:26
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.

3 participants