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

revamp cubemap code and fix quake3 skybox with small bottom face, fix #142 #179

Merged
merged 3 commits into from Mar 10, 2019

Conversation

4 participants
@illwieckz
Copy link
Member

illwieckz commented Mar 5, 2019

Skybox code revamp

The skybox code was very ugly with 10 gotos and buggy behavior, for example it would not attempt to load any multifile skybox if a single-file skybox was successfully loaded but had invalid content.

Also, the code was inefficient, for example it was doing the image rotation operation for every face even if the rotation has an angle of 0°, hence allocating a new image, copying all the pixels in the new image et. for nothing.

Irregular skybox fixing

Some quake3 skyboxes were shipped with a smaller bottom face, for example a 16×16 bottom face while other faces were 512×512 that was probably a trick to save file size when textures were stored losslessly in a tga and bandwith was low as voice modems.

To fix this, a new R_Resize function is added to tr_image.cpp that is just a wrapper to ResampleTexture that makes in-place resizing easier.

The faces are resized to the size of a square that has for width and height the greater width and height found in any face. This way it would not only fix the quake3 skyboxes with smaller bottom faces, but also non-square skyboxes.

A warning is displayed so the user is encouraged to fix the skybox because 1. we workaround something that is broken, 2. user may is likely to have access to tools with resize algorithms that produce better looking results.

In any way, those kind of small bottom faces were likely to not be seen, that's also why they were downscaled at first.

@illwieckz illwieckz force-pushed the illwieckz:fixskybox branch from 476ead5 to 417d797 Mar 5, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 5, 2019

Example of such skybox (alamo, from the Smokin' Guns game):

smaller face in alamo skybox

Before:

broken alamo skybox

After:

fixed alamo skybox

fixed alamo skybox

@illwieckz illwieckz added this to To do in Smokin' Guns via automation Mar 5, 2019

@illwieckz illwieckz added the regression label Mar 5, 2019

@illwieckz illwieckz added the Renderer label Mar 5, 2019

@illwieckz illwieckz force-pushed the illwieckz:fixskybox branch from 417d797 to 340c748 Mar 5, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 5, 2019

Note that this issue was first faced with Tremulous maps so the issue is very likely to occur because that was a supported feature of Tremulous, hence the regression label.

@g-bougard

This comment has been minimized.

Copy link

g-bougard commented Mar 5, 2019

Hi @illwieckz
the fix seems fine to me.
Btw you should not mix tab/space indentation in your patch.

@megatog615

This comment has been minimized.

Copy link

megatog615 commented Mar 5, 2019

Why not examine the sizes of all sides and scale them all to the biggest just in case there exists a skybox with this trick done on another side than the bottom? You could even have the algorithm skip sides that already equal the detected size.

@illwieckz illwieckz force-pushed the illwieckz:fixskybox branch from 340c748 to 0af6d08 Mar 5, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 5, 2019

oh right, there was some boring spaces, fixed!

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 5, 2019

@megatog615 I thought about it but it would require some extra code for an occurrence that will probably never happen… probably not worth it.

@illwieckz illwieckz force-pushed the illwieckz:fixskybox branch from 0af6d08 to 5961af5 Mar 5, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 5, 2019

I just thought about a way to check on all faces while not being so much intrusive so I added a commit trying that instead.

By the way even if it may look possible to do this for all kind of supported cubemap, I really doubt this kind of feature exists outside of quake3.

@illwieckz illwieckz force-pushed the illwieckz:fixskybox branch from 5961af5 to f21da73 Mar 6, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 6, 2019

I was bored by the skybox code so I revamped it entirely. The skybox code was very ugly with 10 gotos and buggy behavior, for example it would not attempt to load any multi-file skybox if a single-file skybox was successfully loaded but had invalid content.

Also, the code was inefficient, for example it was doing the image rotation operation for every face even if the rotation had an angle of 0, hence allocating a new image, copying all the pixels in the new image etc. for nothing.

The skybox faces are now resized to the size of a square that has for width and height the greater width and height found among the six faces of the cube. This way it would not only fix the quake3 skyboxes with smaller bottom faces, but also non-square skyboxes.

@illwieckz illwieckz force-pushed the illwieckz:fixskybox branch from f21da73 to 7e04f8d Mar 6, 2019

@illwieckz illwieckz changed the title fix quake3 skybox with small bottom face, fix #142 revamp cubemap code and fix quake3 skybox with small bottom face, fix #142 Mar 6, 2019

@illwieckz illwieckz force-pushed the illwieckz:fixskybox branch from 7e04f8d to 3058765 Mar 6, 2019

@slipher

slipher approved these changes Mar 6, 2019

Copy link
Contributor

slipher left a comment

We no longer have to live in fear: the goto's are vanquished!

Show resolved Hide resolved src/engine/renderer/tr_image.cpp Outdated
Show resolved Hide resolved src/engine/renderer/tr_image.cpp Outdated
@@ -1937,23 +1937,53 @@ static void R_Rotate( byte *in, int width, int height, int degrees )
ri.Hunk_FreeTempMemory( tmp );
}

byte *R_Resize( byte *in, int width, int height, int width2, int height2 )

This comment has been minimized.

@slipher

slipher Mar 6, 2019

Contributor

This could use a comment or a different name to indicate that it's specifically for upsampling, not any resize.

This comment has been minimized.

@illwieckz

illwieckz Mar 6, 2019

Author Member

do you think that algorithm does not work for downscaling?
we only use it to upscale, but isn't it generic enough to downscale too?

This comment has been minimized.

@slipher

slipher Mar 7, 2019

Contributor

Well maybe, if not with the best quality. But there's already a downsampling function ResampleTexture in the same file.

@illwieckz illwieckz force-pushed the illwieckz:fixskybox branch 2 times, most recently from 55afc66 to 6e3792d Mar 6, 2019

@illwieckz illwieckz force-pushed the illwieckz:fixskybox branch 2 times, most recently from b114737 to 1d45c0c Mar 9, 2019

@illwieckz illwieckz force-pushed the illwieckz:fixskybox branch from 1d45c0c to 58342ad Mar 10, 2019

@illwieckz illwieckz merged commit 58342ad into DaemonEngine:master Mar 10, 2019

2 checks passed

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

Smokin' Guns automation moved this from To do to Done Mar 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.