Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Control panel for simulations #443
base: master
Are you sure you want to change the base?
Control panel for simulations #443
Changes from 23 commits
1d769c0
be39c0c
1a160a4
ce2d52e
8992466
22cb37f
033ac59
5db885f
8f649f3
6ebd496
9f5a3fd
875640b
7408f8f
2149bef
b9fcedc
c2e1aa0
3ae3215
50ba064
b099a3b
0d3256e
b0be177
5a4cc26
21f8b50
d0faa5c
e4f3f2a
ca489f3
2aac8ac
c9abe28
8ba6455
e3db915
6be2bb2
892c1d0
0ef6beb
7bab656
9179f2e
30e0ed1
9927976
30ae1ca
e65067c
b150900
359be34
167ce5b
4deacf4
6083037
eae6c72
a656377
414e349
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Mention the README.md or somewhere else where the design pattern is explained
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.
If we're destroying it, maybe take as an R-value reference?
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.
// namespace emp::prefab
(here and on other namespaces)
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.
// #ifndef EMP_BUTTON_GROUP_HPP
(here and all the other files)
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.
I wouldn't indent the internal namespace
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.
unordered_map
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.
explain what they take as a param and what they return
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.
hmm... it looks like you're only allowing one refresh checker at a time?
What I would do is make two functor classes as public members of this class
ControlPanel::MillisecondRefresher
ControlPanel::FrameRefresher
Both functor classes would take a value as their constructor
instead of storing refresh_rates, refrhesh_units, and refresh_checkers
I would just store a std::function refresh_checker
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.
I would just copy/paste those comments here
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.
Explain that by simulation you mean a void callback that can be used to advance the simulation one frame
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.
Should this be private? Who needs to use this outside of the class?
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.
The
ControlPanel
class needs access.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 yup makes sense!
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.
briefly explain what these members are here for
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.
good accessibility move
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.
I would name this step_sim, or update_sim
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.
rename do_redraw should_redraw
I might also move auto& wid : refresh_list wid.Redraw to a helper method (probably private?) called do_redraw
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.
or DoRedraw
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.
const bool
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.
SetSimulationUpdateCallback
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.
Why do outside users need to access this? delete or make private?
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.
I would have this take a std::function and note that they can write their own or use ControlPanel::MillisecondRefresher or ControlPanel::FrameRefresher
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.
Is there a definite use case for public access to this? We can make it private and then always make it public later
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.
does this need to be a ButtonGroup immediately preceding?
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.
There's already a button group to hold the start/stop toggle and the step button so any buttons streamed get added there until a new button group gets streamed in.
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.
std::forward<IN_TYPE> ? (unless you have a good reason to deviate from that idiom)