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

Fixed frame overflow in DaggerfallMobileUnit. #2571

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

KABoissonneault
Copy link
Collaborator

Fixes for #2569.

I found an issue where archive 288 has 4 frames for record 0, 1, 2, and 4, but 8 frames for record 3. Record 3 is the "back diagonal" sprites (both left and right). Normally, all mobile archives have the same number of frames on each orientation, but the Ancient Lich is an exception. When changing orientation, the currentFrame is preserved so that the animation keeps playing from where it was. However, for the Ancient Lich, if you change orientation from the back diagonal while on frame 4, 5, 6, or 7 is playing, the currentFrame will now be past the allowed frames for the new orientation (back or side). On unmodded DFU, this reaches into a "Casting spell" frame in summary.AtlasRect (because rectIndex has enough room to overflow into the next records, rather than go out of bounds of the AtlasRect). When using texture replacements from mods (ex: Vanilla Enhanced), you get the exception shown in #2569 (because we don't use the atlas anymore).

The fix is on line 371 - I simply reset currentFrame to 0 if the frame is not valid for the new orientation. A bit jarring, but better than the current behavior.

The rest of the changes are more bounds checking and error logs for these issues. I find DaggerfallMobileUnit has lots of moving parts and lots of arrays to go out of bounds, so it helps to have better logs there.

The output looks like this:

image

Hopefully this will help diagnose future issues with mobiles, if any remain. In any case, with mods like Daggerfall Enemy Expansion, it's easy to accidentally create invalid MobileEnemy data that creates random errors in DaggerfallMobileUnit, and this will help mod development.

@petchema
Copy link
Collaborator

petchema commented Feb 3, 2024

It's hard to talk about a "best solution" without knowing the original intent (and checking what's going on in Daggerfall classic will probably be difficult), but at least for the record, since both 4 and 8 frame cycles picture a single walking cycle (left foot forward, right foot forward), another solution would be to apply cross-multiplication when orientation changes

--- a/Assets/Scripts/Internal/DaggerfallMobileUnit.cs
+++ b/Assets/Scripts/Internal/DaggerfallMobileUnit.cs
@@ -328,6 +328,8 @@ namespace DaggerfallWorkshop
             int orientation = - Mathf.RoundToInt(enemyFacingAngle / anglePerOrientation);
             // Wrap values to 0 .. numberOrientations-1
             orientation = (orientation + numberOrientations) % numberOrientations;
+            if (lastOrientation >= 0)
+                currentFrame = currentFrame * summary.StateAnims[orientation].NumFrames / summary.StateAnims[lastOrientation].NumFrames;
 
             // Change enemy to this orientation
             if (orientation != lastOrientation)

// If you change orientation from a back diagonal during frame 4, 5, 6, or 7, you overflow the other anims, where only 0 to 3 are valid
if (lastOrientation >= 0)
{
currentFrame = currentFrame * summary.StateAnims[orientation].NumFrames / summary.StateAnims[lastOrientation].NumFrames;
Copy link
Owner

Choose a reason for hiding this comment

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

This is really clever, I like this

@KABoissonneault KABoissonneault merged commit 3a32889 into Interkarma:master Feb 7, 2024
@KABoissonneault KABoissonneault deleted the fix/mobile-error branch February 7, 2024 03:56
This was referenced Feb 7, 2024
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.

4 participants