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 https://github.com/beyond-all-reason/spring/issues/1552 #1603

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lhog
Copy link
Collaborator

@lhog lhog commented Jul 17, 2024

No description provided.

if (groundDecals == &nullDecalDrawer) {
groundDecals = GetInstance();
}
if (groundDecals != &nullDecalDrawer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps just SetDecalLevel(decalLevel);?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah

@@ -13,6 +13,8 @@ class IGroundDecalDrawer
CR_DECLARE(IGroundDecalDrawer)
public:
static bool GetDrawDecals() { return (decalLevel > 0); }
static int GetDecalLevel() { return decalLevel; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this return std::abs(decalLevel)? Since level +x means x, but -x also means x, just currently disabled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I was going through this I was inclined to remove altogether or just make it binary.
The current approach is inherited that slightly crazy idea of controlling on and off states through the sign bit.

Copy link
Collaborator

@sprunk sprunk Jul 19, 2024

Choose a reason for hiding this comment

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

I was inclined to remove altogether or just make it binary.

Yeah maybe the current "multiplier for decal duration" doesn't make much sense (see also #1476). But some way to control how many decals there can be is good imo (as a performance vs visuals tradeoff knob, since a battlefield littered with scars is cool which is the original reason to extend decal duration).

If not touching the design then perhaps best to just be clean and have separate bool and (u?)int instead of doing bit hax to save 4 bytes. Anyway, current is also fine (since it's just used by the debug print in the /decals command).

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.

2 participants