-
Notifications
You must be signed in to change notification settings - Fork 177
Progress bar helper class #1467
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thank you very much for this PR! Left a few comments & fix suggestions, but looking good overall :)
Ref<CCSprite> m_progressBar = nullptr; // Progress bar outline | ||
CCSprite* m_progressBarFill = nullptr; // Progress bar fill | ||
|
||
float m_progress = 0.f; // Current progress bar fill percentage ranging from 0 to 100 | ||
|
||
float m_progressBarFillMaxWidth = 0.f; // Max width for the progress fill bar node | ||
float m_progressBarFillMaxHeight = 0.f; // Max height for the progress fill bar node |
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.
Class members should be PIMPL; aka these should be behind a source-defined Impl
class and std::unique_ptr<Impl> m_impl
. See other Geode classes for examples
appreciate the feedback! i've made adjustments based on what you mentioned and asked for - lmk if anything else could use further tweaking :D |
* | ||
* @param color RGB color object | ||
*/ | ||
void setProgressBarFillColor(ccColor3B color); |
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.
personally just setFillColor
is enough
/** | ||
* Get the current style of the progress bar | ||
*/ | ||
ProgressBarStyle getProgressBarStyle() const; |
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 just be getStyle
, like how setStyle
is
loader/src/ui/nodes/ProgressBar.cpp
Outdated
Ref<CCSprite> progressBar = nullptr; // Progress bar outline | ||
CCSprite* progressBarFill = nullptr; // Progress bar fill | ||
CCLabelBMFont* progressPercentLabel = nullptr; // The text label displaying the percentage | ||
|
||
float progress = 0.0f; // Current progress bar fill percentage ranging from 0 to 100 | ||
|
||
ccColor3B progressBarFillColor = { 255, 255, 255 }; // Current color of the filled progress bar | ||
bool showProgressPercentLabel = false; // Whether to show the label showing the percentage of the current progress | ||
|
||
ProgressBarStyle style = ProgressBarStyle::Level; // Style of the progress bar | ||
|
||
float progressBarFillMaxWidth = 0.0f; // Max width for the progress fill bar node | ||
float progressBarFillMaxHeight = 0.0f; // Max height for the progress fill bar node |
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.
imo comments should be on the line before, not at the end of the line
loader/src/ui/nodes/ProgressBar.cpp
Outdated
m_impl->style = style; | ||
|
||
this->removeAllChildren(); | ||
init(); // init again with new style |
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.
calling CCNode::init multiple times 😟
sorry about all that, hopefully this fixes it! |
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.
Looks good to me, don't have any major complaints, just some styling suggestions! Tyvm for this PR!
std::unique_ptr<Impl> m_impl; | ||
|
||
ProgressBar(); | ||
~ProgressBar(); |
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 destructor should be marked virtual
loader/src/ui/nodes/ProgressBar.cpp
Outdated
case ProgressBarStyle::Level: | ||
m_impl->progressBar = CCSprite::create("slidergroove2.png"); | ||
m_impl->progressBar->setID("progress-bar"); | ||
m_impl->progressBar->setAnchorPoint({ 0.5, 0.5 }); | ||
m_impl->progressBar->setPosition({ m_impl->progressBar->getScaledContentWidth() / 2.0f, m_impl->progressBar->getScaledContentHeight() / 2.0f }); | ||
m_impl->progressBar->setZOrder(1); | ||
|
||
m_impl->progressBarFill = CCSprite::create("sliderBar2.png"); | ||
m_impl->progressBarFill->setID("progress-bar-fill"); | ||
m_impl->progressBarFill->setAnchorPoint({ 0, 0.5 }); | ||
m_impl->progressBarFill->setPosition({ 2.0f, m_impl->progressBar->getScaledContentHeight() / 2.0f }); | ||
m_impl->progressBarFill->setColor(m_impl->progressBarFillColor); | ||
m_impl->progressBarFill->setZOrder(-1); | ||
|
||
m_impl->progressBarFillMaxWidth = m_impl->progressBar->getScaledContentWidth() - 4.0f; | ||
m_impl->progressBarFillMaxHeight = m_impl->progressBarFill->getScaledContentHeight() - 0.5f; | ||
|
||
m_impl->progressPercentLabel = CCLabelBMFont::create("0%", "bigFont.fnt"); | ||
m_impl->progressPercentLabel->setID("progress-percent-label"); | ||
m_impl->progressPercentLabel->setScale(0.5f); | ||
m_impl->progressPercentLabel->setAnchorPoint({ 0, 0.5 }); | ||
m_impl->progressPercentLabel->setPosition({ m_impl->progressBar->getScaledContentWidth() + 2.5f, m_impl->progressBar->getScaledContentHeight() / 2.0f }); | ||
m_impl->progressPercentLabel->setAlignment(CCTextAlignment::kCCTextAlignmentLeft); | ||
m_impl->progressPercentLabel->setVisible(m_impl->showProgressPercentLabel); | ||
m_impl->progressPercentLabel->setZOrder(1); |
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.
This is just a styling thing but I would indent this whole thing one tab and then surround the block with braces, like so:
switch (x) {
case y: {
// code here...
} break;
}
ProgressBar(); | ||
~ProgressBar(); | ||
|
||
void customSetup(); |
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 tend to call functions like this reload
or reloadStyle
instead to make it clear that it may (and should) be called multiple times
loader/src/ui/nodes/ProgressBar.cpp
Outdated
}; | ||
|
||
if (m_impl->progressPercentLabel) { | ||
auto percentString = fmt::format("{}%", static_cast<int>(m_impl->progress)); |
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.
This should probably use geode::utils::numToString
with a precision of 0. The class should probably also have a way to customize the precision of the percentage
happy to contribute! hopefully i've made this as best as it could possibly be! |
Since the base game doesn't use a class to create its progress bar UI, I would like to propose a helper class that I wrote that can help developers easily re-create them without having to use and set up the sprites and set the texture rects themselves.
ProgressBar
This class creates a progress bar. It can mimic the style of the progress bar shown while playing a level, or the style of the Normal/Practice mode progress bars shown when looking at your current best progress in the level. It inherits
cocos2d::CCNode
.Example
progresstest.mp4