Remove hard-coded framecount for linking progress animation. #628
Conversation
| { | ||
| std::vector<ST::string> names; | ||
|
|
||
| for (auto resource : ClientResources) |
There was a problem hiding this comment.
I believe that this makes an unnecessary copy of the string. We may prefer const auto&
There was a problem hiding this comment.
I wish I were better at seeing these myself.
| fImageRotation[i] = ST::format("xLoading_Linking.{02}.png", i); | ||
| // Find linking-animation frame IDs and store the sorted list | ||
| std::regex re("xLoading_Linking\\.[0-9]+?\\.png"); | ||
| for (auto name : plClientResMgr::Instance().getResourceNames()) { |
There was a problem hiding this comment.
I believe that this makes an unnecessary copy of the string. We may prefer const auto&
| // Find linking-animation frame IDs and store the sorted list | ||
| std::regex re("xLoading_Linking\\.[0-9]+?\\.png"); | ||
| for (auto name : plClientResMgr::Instance().getResourceNames()) { | ||
| if (std::regex_match(name.to_std_string(), re)) |
There was a problem hiding this comment.
You can avoid the conversion to std::string here by using std::regex_match(name.begin(), name.end(), re) instead
There was a problem hiding this comment.
Excellent, I didn't see that option when I was looking at the regex matches. I was hoping to write something a bit better here.
| fImageRotation[i] = ST::format("xLoading_Linking.{02}.png", i); | ||
| // Find linking-animation frame IDs and store the sorted list | ||
| std::regex re("xLoading_Linking\\.[0-9]+?\\.png"); | ||
| for (auto name : plClientResMgr::Instance().getResourceNames()) { |
There was a problem hiding this comment.
const auto& name to avoid copying...
There was a problem hiding this comment.
I wish I were better at seeing these myself.
| { | ||
| std::vector<ST::string> names; | ||
|
|
||
| for (auto resource : ClientResources) |
There was a problem hiding this comment.
const auto& resource to avoid copies (if applicable)
| return fImageRotation[index]; | ||
| else | ||
| return fImageRotation[0]; | ||
| return fImageRotation[index % (fImageRotation.size() - 1)]; |
There was a problem hiding this comment.
% doesn't protect against negative indexes... I'd suggest either making this unsigned or keeping the old behavior (with the non-hard-coded size)
There was a problem hiding this comment.
I must have been thinking of this backwards. Thanks.
There was a problem hiding this comment.
The old method doesn't protect against negatives, either. Yikes.
As far as I can tell, the code at both ends of the function call are checking that we don't exceed our bounds, and there should never be a reason to play the animation in reverse. But I'd rather it be solid against such a usage.
There was a problem hiding this comment.
Is std::clamp available for our usage? Would that be appropriate?
There was a problem hiding this comment.
std::clamp was added in C++17--we are still using C++14.
There was a problem hiding this comment.
Ah, that's out then. :)
There was a problem hiding this comment.
The old method doesn't protect against negatives, either. Yikes.
Ah, good point... Maybe not a big deal then.
Deledrius
force-pushed
the
smart_loading_anim
branch
from
a14b9cf
to
7076fe5
Compare
This fixes a long-standing crash which could occur if using a different
resource.datthan one the client was hard-coded to expect. Previously, the number of frames was dictated at compile-time. This change allows the plProgressMgr to query for the list of resources, find the ones matching the given pattern, and subsequently use that sorted list as the IDs from which it builds and cycles the animation.