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 RA2 lobrdb sprites not getting loaded #13882

Merged
merged 2 commits into from Oct 7, 2017

Conversation

Projects
None yet
5 participants
@Mailaender
Member

Mailaender commented Aug 21, 2017

Closes #12115.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Aug 21, 2017

Contributor

Is this intends to fix the "OpenRA-calls-SHP(TS)-files-invalid-if-they-start-with-an-empty-frame" bug?

Contributor

GraionDilach commented Aug 21, 2017

Is this intends to fix the "OpenRA-calls-SHP(TS)-files-invalid-if-they-start-with-an-empty-frame" bug?

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Aug 21, 2017

Member

Maybe. The RA2 low bridge tiles come with an empty 1st frame and will now be detected as valid ShpTS:

lobrdb01.sno
w 0 h 0 type 4
w 120 h 70 type 3

while it was unknown bits read before, as we didn't actually scan the image headers, but something else. I suggest you give it a try. It might fix your problem as well. This patch was generalized during review.

Member

Mailaender commented Aug 21, 2017

Maybe. The RA2 low bridge tiles come with an empty 1st frame and will now be detected as valid ShpTS:

lobrdb01.sno
w 0 h 0 type 4
w 120 h 70 type 3

while it was unknown bits read before, as we didn't actually scan the image headers, but something else. I suggest you give it a try. It might fix your problem as well. This patch was generalized during review.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 7, 2017

Member

Is this intends to fix the "OpenRA-calls-SHP(TS)-files-invalid-if-they-start-with-an-empty-frame" bug?

The current version of the PR should fix that, yes.

Member

pchote commented Sep 7, 2017

Is this intends to fix the "OpenRA-calls-SHP(TS)-files-invalid-if-they-start-with-an-empty-frame" bug?

The current version of the PR should fix that, yes.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 10, 2017

Contributor

Can someone provide a test-case shp? I don't feel like downloading RA2 just for this (I have it on Origin, but not currently installed).

Contributor

reaperrr commented Sep 10, 2017

Can someone provide a test-case shp? I don't feel like downloading RA2 just for this (I have it on Origin, but not currently installed).

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Sep 10, 2017

Contributor

infshdw as the RA2 Rocketeer shadow for first-frame-empty bug, ngmisl is for the all-frames-empty extreme case.

shptstest.zip

Contributor

GraionDilach commented Sep 10, 2017

infshdw as the RA2 Rocketeer shadow for first-frame-empty bug, ngmisl is for the all-frames-empty extreme case.

shptstest.zip

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 10, 2017

Contributor

Thanks.

infshdw works fine with this, ngmisl still triggers a "tried to read beyond stream" error:
https://gist.github.com/reaperrr/4ca14b9dfd8303659fa9e913f5a10965

Since it's an entirely empty sprite, I'd say as long as it doesn't crash the game and just silently logs an error, it's probably fine.

I'll wait for @pchote's opinion before merging this.

Contributor

reaperrr commented Sep 10, 2017

Thanks.

infshdw works fine with this, ngmisl still triggers a "tried to read beyond stream" error:
https://gist.github.com/reaperrr/4ca14b9dfd8303659fa9e913f5a10965

Since it's an entirely empty sprite, I'd say as long as it doesn't crash the game and just silently logs an error, it's probably fine.

I'll wait for @pchote's opinion before merging this.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Sep 10, 2017

Member

You can also test via OpenRA/ra2#393 which references this pull request. If you translate https://github.com/OpenRA/ra2/blob/master/fetch-content.sh into PowerShell, it should be convenient to test as well without Origin or CD drives.

Member

Mailaender commented Sep 10, 2017

You can also test via OpenRA/ra2#393 which references this pull request. If you translate https://github.com/OpenRA/ra2/blob/master/fetch-content.sh into PowerShell, it should be convenient to test as well without Origin or CD drives.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 10, 2017

Member

Since it's an entirely empty sprite, I'd say as long as it doesn't crash the game and just silently logs an error, it's probably fine.

The code is actually still wrong, the f++ should be ++f for the loop to terminate at the right time.
This then leads to a second set of crashes from frames that are completely empty but which we still use ingame for some reason (null.shp, lobrdg28.tem) and which now fail to be matched as a shp.

Member

pchote commented Sep 10, 2017

Since it's an entirely empty sprite, I'd say as long as it doesn't crash the game and just silently logs an error, it's probably fine.

The code is actually still wrong, the f++ should be ++f for the loop to terminate at the right time.
This then leads to a second set of crashes from frames that are completely empty but which we still use ingame for some reason (null.shp, lobrdg28.tem) and which now fail to be matched as a shp.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 16, 2017

Member

Following on with this... We can avoid the crash I refer to above by being a bit smarter about the checking. If we are able to identify the frame types that are used with zero-sized sprites then we can have an explicit check to allow these all-empty sprites to be accepted without false-matching against all-zero data.

Member

pchote commented Sep 16, 2017

Following on with this... We can avoid the crash I refer to above by being a bit smarter about the checking. If we are able to identify the frame types that are used with zero-sized sprites then we can have an explicit check to allow these all-empty sprites to be accepted without false-matching against all-zero data.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 16, 2017

Member

Try the following diff to fix ngmisl:

diff --git a/OpenRA.Mods.Common/SpriteLoaders/ShpTSLoader.cs b/OpenRA.Mods.Common/SpriteLoaders/ShpTSLoader.cs
index a89949eab8..d3f5e79ada 100644
--- a/OpenRA.Mods.Common/SpriteLoaders/ShpTSLoader.cs
+++ b/OpenRA.Mods.Common/SpriteLoaders/ShpTSLoader.cs
@@ -114,12 +114,17 @@ bool IsShpTS(Stream s)
                                w = s.ReadUInt16();
                                h = s.ReadUInt16();
                                type = s.ReadUInt8();
+
+                               // Zero sized frames always define a non-zero type
+                               if ((w == 0 || h == 0) && type == 0)
+                                       return false;
+
                                s.Position += 19;
                        }
-                       while (w == 0 && h == 0 && f++ < imageCount);
+                       while (w == 0 && h == 0 && ++f < imageCount);
 
                        s.Position = start;
-                       return type < 4;
+                       return f == imageCount || type < 4;
                }
 
                ShpTSFrame[] ParseFrames(Stream s)
Member

pchote commented Sep 16, 2017

Try the following diff to fix ngmisl:

diff --git a/OpenRA.Mods.Common/SpriteLoaders/ShpTSLoader.cs b/OpenRA.Mods.Common/SpriteLoaders/ShpTSLoader.cs
index a89949eab8..d3f5e79ada 100644
--- a/OpenRA.Mods.Common/SpriteLoaders/ShpTSLoader.cs
+++ b/OpenRA.Mods.Common/SpriteLoaders/ShpTSLoader.cs
@@ -114,12 +114,17 @@ bool IsShpTS(Stream s)
                                w = s.ReadUInt16();
                                h = s.ReadUInt16();
                                type = s.ReadUInt8();
+
+                               // Zero sized frames always define a non-zero type
+                               if ((w == 0 || h == 0) && type == 0)
+                                       return false;
+
                                s.Position += 19;
                        }
-                       while (w == 0 && h == 0 && f++ < imageCount);
+                       while (w == 0 && h == 0 && ++f < imageCount);
 
                        s.Position = start;
-                       return type < 4;
+                       return f == imageCount || type < 4;
                }
 
                ShpTSFrame[] ParseFrames(Stream s)

@pchote pchote dismissed their stale review Sep 16, 2017

Changes requested.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Sep 16, 2017

Member

Updated and confirmed that OpenRA/ra2#393 still works as well as the RA2 infshdw and ngmisl as well as TS null.shp and lobrdg28.

Member

Mailaender commented Sep 16, 2017

Updated and confirmed that OpenRA/ra2#393 still works as well as the RA2 infshdw and ngmisl as well as TS null.shp and lobrdg28.

@reaperrr

Works with all test files now 👍

@abcdefg30 abcdefg30 merged commit 5b16bb9 into OpenRA:bleed Oct 7, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Oct 7, 2017

@Mailaender Mailaender deleted the Mailaender:shpts-type18 branch Oct 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment