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

Fix glitch of burning building animation #639

Merged
merged 13 commits into from
Mar 22, 2024
Merged

Conversation

zzam
Copy link
Contributor

@zzam zzam commented Mar 21, 2024

Burning animation disappears before building is completely destroyed.
Fix and add unittest.
Simplify storage a bit.
Comment: Percent logic is similar to construction. Code could be combined.

State before:
Burning at 6/400=1.5%:
image

No longer burning at 2/400=0.5%:
image

Make BurningBuildingFrame a vector of objects instead of smart-pointers.
CclDefineBurningBuilding: Put elements directly into correct position in vector.
Fixes fire stopping to burn when hitpoints reach zero percent.
tests/stratagus/test_missile_fire.cpp Outdated Show resolved Hide resolved
tests/stratagus/test_missile_fire.cpp Outdated Show resolved Hide resolved
tests/stratagus/test_missile_fire.cpp Show resolved Hide resolved
@@ -323,6 +323,12 @@ namespace ranges
std::sort(std::begin(range), std::end(range), std::forward<Comparer>(comparer));
}

template <typename Range, typename Comparer = std::less<>>
bool is_sorted(Range &range, Comparer &&comparer = {})
Copy link
Contributor

Choose a reason for hiding this comment

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

const Range.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if you are familiar enough, projection (see above typename Proj = identity for find)

So we would have:

bool is_BurningBuildingFramesSorted()
{
	return range::is_sorted(BurningBuildingFrames, std::greater<>{}, &Frame::Percent);
}

Copy link
Contributor Author

@zzam zzam Mar 21, 2024

Choose a reason for hiding this comment

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

I have seen projections, but is that possible with c++17 ?

Update: I will have a look at "find". I did not get that you referred to an existing implementation in util.h

Copy link
Contributor

@Jarod42 Jarod42 Mar 21, 2024

Choose a reason for hiding this comment

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

I mean like that:

template <typename Range, typename Comparer = std::less<>, typename Proj = identity>
bool is_sorted(const Range &range, Comparer &&comparer = {}, Proj &&proj = {}) {
    return std::is_sorted(std::begin(range), std::end(range),
                          [&](const auto& lhs, const auto& rhs){
                              return comparer(std::invoke(proj, lhs), std::invoke(proj, rhs);
                          });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That part I got rather quickly after I found the code above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Just needed to use a lambda for the projection (Otherwise the code would not be generic, or ugly)
e.g.

typedef typename std::iterator_traits<decltype(range.begin())>::value_type T;
...
then use &T::Percent

Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly because of intermediate IsPercentDecreasingValid (used only once).
If code is inlined/moved directly in IsBurningBuildingFramesValid(), it would be possible.

Copy link
Contributor Author

@zzam zzam Mar 22, 2024

Choose a reason for hiding this comment

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

Sure.
I still wonder if we should combine the percentage-lookup of construction and burning?
If yes, it should still be in a separate PR, I think.

That would require the code to be generic/put in header.
How should the signature of such a percentage-lookup function look like?

src/missile/missile.cpp Outdated Show resolved Hide resolved
@@ -1172,7 +1172,8 @@ int ViewPointDistanceToMissile(const Missile &missile)
*/
MissileType *MissileBurningBuilding(int percent)
{
for (auto &frame : BurningBuildingFrames) {
for (int i = BurningBuildingFrames.size() - 1; i >= 0; --i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Dangerous to mix signed/unsigned like that.
But changed afterward :-)

@Jarod42 Jarod42 merged commit 6aa6d81 into Wargus:master Mar 22, 2024
3 checks passed
@zzam zzam deleted the burningBuilding branch March 22, 2024 19:54
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.

None yet

3 participants