[Studio] Bone Tool cleanup #2325
Conversation
|
@rodolforg Code looks good to me. @morevnaproject Can you test this PR? |
rodolforg
force-pushed
the
studio-bone-tool-cleanup
branch
from
723a299
to
06ea134
Compare
Added to my list now. Thank you! ^__^ |
|
@morevnaproject I suggest you to write/record your tests here, because I'll submit another PR related to Bone Tool following this PR approval XD |
| } else if (Layer_SkeletonDeformation::Handle::cast_dynamic(layer)) { | ||
| std::vector<std::pair<Bone,Bone>> bone_pair_list = (*list_desc.get_value_node())(get_canvas()->get_time()).get_list_of(std::pair<Bone,Bone>()); | ||
| for (const auto& item : bone_pair_list) | ||
| bone_list.push_back(item.second); |
There was a problem hiding this comment.
After reviewing this part I have realized that there is a bug in original code: it should use "first" bone instead of "second". Because of this bug I was experiencing an issue when it was not possible to select bone as active at some when second bone had different position than second (this usually happens when user deforms image).
So, we should change this part of code from "item.second" to "item.first', but I suggest to do that in separate PR, because it's better to have this PR for cleanup&refactoring only. ^__^
There was a problem hiding this comment.
First item is for resting state and the second one is for posing state, right?
There was a problem hiding this comment.
Maybe it depends on layer activate status?
There was a problem hiding this comment.
First item is for resting state and the second one is for posing state, right?
Exactly!
There was a problem hiding this comment.
Maybe it depends on layer activate status?
In general case - yes. But Bone Tool always sets the Skeleton Deformation layer to “inactive” state. So, in our case we always deal with “first”.
Woops, I already did my tests before I saw your message! This is a great idea and I will do that next time (I believe we have many more fixes ahead XD). |
|
The PR is looks good to me! Thank you! |
rodolforg
force-pushed
the
studio-bone-tool-cleanup
branch
from
06ea134
to
5c969e9
Compare
Just for safety. The methods would crash with a null pointer, although it should never be null there.
morevnaproject
force-pushed
the
studio-bone-tool-cleanup
branch
from
5c969e9
to
8706229
Compare
Broken due to PR synfig#2325
Broken due to PR #2325
No description provided.