Page MenuHomeWildfire Games

Removes hardcoded SkyBox sizes and use infinity sky
ClosedPublic

Authored by vladislavbelov on Nov 28 2018, 11:51 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22039: Remove hardcoded SkyBox sizes and use the infinity sky. No we render
Trac Tickets
#3458
Summary

We have a bunch of problems with sky:

  • Wrong coordinates in reflections
  • Intersections with landscape (D1401)
  • Hard-coded size values.

The patch solves all these problems. It adds only one issue: the sky is in infinity. So all clouds move together with the camera. I suppose, that it's not the real issue, because a size of a map much smaller than the distance to clouds in real world. So it's correct to use infinity.

It solves the problem mentioned by @elexis in D1401, that all maps are different and have different sizes.

Test Plan
  1. Apply the patch and compile the game
  2. Check few maps with water, that reflections are correct.
  3. Check that no bad artifacts on horizons.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Vulcan added a subscriber: Vulcan.Nov 28 2018, 11:57 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/804/

lyv added a subscriber: lyv.Nov 29 2018, 7:56 AM
lyv added inline comments.
source/renderer/SkyManager.cpp
119 ↗(On Diff #7010)

(That does not look good. Same with path2. But thats out of scope.)

vladislavbelov marked an inline comment as done.Nov 29 2018, 8:19 AM
vladislavbelov added inline comments.
source/renderer/SkyManager.cpp
119 ↗(On Diff #7010)

Do you mean hardcoded extension?

lyv added inline comments.Nov 29 2018, 8:24 AM
source/renderer/SkyManager.cpp
119 ↗(On Diff #7010)

We ought to use CCacheLoader, no?
At least thats how other textures seems to be loaded.

vladislavbelov marked an inline comment as done.Nov 29 2018, 8:42 AM
vladislavbelov added inline comments.
source/renderer/SkyManager.cpp
119 ↗(On Diff #7010)

We need to use TextureManager (it uses CCacheLoader), but there were few related issues.

the sky is in infinity. So all clouds move together with the camera

If you mean the clouds in the sky texture, that doesn't seem like a problem.
If we want closer clouds, they should be separate from the skybox probably: #46.

Wrong coordinates in reflections

That's the TerrainRenderer 0.3? Where does it come from, what was the intention behind it?

Was wondering if this could be split into multiple diffs, but doesn't look like it.

Looks good.

source/renderer/SkyManager.cpp
119 ↗(On Diff #7010)

Which issues?

(Hardcoding .dds and .dds.cached.dds sounds problematic, as usually only the png image exists until bundling (but apparently it's not a problem currently).)

244 ↗(On Diff #7010)

//Translate so the sky center is rendered at the origin?

elexis updated the Trac tickets for this revision.Nov 29 2018, 11:15 AM
vladislavbelov marked 2 inline comments as done.Nov 29 2018, 1:33 PM
In D1683#66749, @elexis wrote:

the sky is in infinity. So all clouds move together with the camera

If you mean the clouds in the sky texture, that doesn't seem like a problem.
If we want closer clouds, they should be separate from the skybox probably: #46.

Yep, clouds in the sky texture. Separate "clouds" we have only as particles. Anyway close clouds should be separate, yes.

Wrong coordinates in reflections

That's the TerrainRenderer 0.3? Where does it come from, what was the intention behind it?

I suppose it was because of +45 degree. But PI / 4 isn't equal to 0.3. So, probably wrong calculations. Especially from // TODO: check that this rotates in the right direction..

source/renderer/SkyManager.cpp
119 ↗(On Diff #7010)

One that you solved in D1583. Second that we shouldn't create a cache version of PNG if DDS with the same properties can't be read (i.e. the power of two problem).

244 ↗(On Diff #7010)

//Translate so the sky center is at the camera space origin then.

Stan added a subscriber: Stan.Dec 26 2018, 12:57 PM
Stan added inline comments.
source/renderer/SkyManager.cpp
119 ↗(On Diff #7010)

Currently you can't use PNG files for skyboxes

vladislavbelov added a subscriber: wraitii.
Stan added a comment.Dec 26 2018, 11:39 PM

Can you post screenshots of say Corinthian Ismuth (4)

vladislavbelov added a comment.EditedDec 27 2018, 1:55 AM
In D1683#67409, @Stan wrote:

Can you post screenshots of say Corinthian Ismuth (4)

Sure:

Stan added a comment.Dec 28 2018, 2:21 PM

Here is a little image comparison left is AFTER right is BEFORE.

It looks good to me the sky is definitely better this way, however it feels like it dulls the image a bit, but It is only really noticeable when looking at the sky itself which doesn't happen often.

Jebel Barkal would also be a good testmap, because the ground level is extremely high, lowering the current global skybox a lot.
Last time I looked at the implementation, it appeared very good and correct. Only the unknown angle 0.3 was an open question.
The screenshots remind me that creators of promotional material also need to disable the shroud of darkness. The patch we used for the alpha 23 trailer should be refactored and committed I guess.
From the artistic pov one could check that the skybox is well enough positioned vertically for the shroud of darkness to match the skybox images. From the screenshots it appears to be the case.

source/renderer/SkyManager.cpp
244 ↗(On Diff #7010)

// Render the skybox at the current camera location, so that the skybox appears indefinitely far away?

source/renderer/TerrainRenderer.cpp
1 ↗(On Diff #7010)

9

wraitii added a comment.EditedJan 3 2019, 7:16 PM

I'm not a fan of this change as-is. Our maps aren't huge but they can get big enough that this looks weird over water for partly cloudy maps to me.

I think it would be better to have two levels of skybox, a "near" which is fixed as now and a "far" which is infinitely far away.

On the 0.3 change, I think that's just empirical from me. The sun wasn't positioned correctly in reflections without it. It might have been a weird side-effect of them being square, so this probably didn't entirely fix the problem and might just have been me misunderstanding what happened, mind you.

Our maps aren't huge but they can get big enough that this looks weird over water for partly cloudy maps to me.
On the 0.3 change, I think that's just empirical from me. The sun wasn't positioned correctly in reflections without it. It might have been a weird side-effect of them being square, so this probably didn't entirely fix the problem and might just have been me misunderstanding what happened, mind you.

Exactly the current implementation has wrong reflections. After the patch it works. Also all our sky textures are designed to be on a cube, not on a transformed cube.

I think it would be better to have two levels of skybox, a "near" which is fixed as now and a "far" which is infinitely far away.

I agree with that, I have few ideas about that.

wraitii accepted this revision.Jan 5 2019, 10:15 AM

Still believe this should lead to a skybox change but honestly svn's skyboxes are already pretty far away so in practice the svn reflections already exhibit the behaviour I describe above.

This patch is otherwise functional, complete and a simplification so go ahead.

This revision is now accepted and ready to land.Jan 5 2019, 10:15 AM
elexis added a comment.Jan 5 2019, 4:02 PM

Thanks for the patch, it was an important bugfix as the wrong skybox was noticeable in multiple maps, see the original ticket, also jebel barkal (which is due to pmp heightmap used in rmgen).
(I didn't verify the water shader math, so I'm not eager to take moral responsibility for possible bugs.)

Current code:
Default angle and zoom level:


Default angle and a bit zoomed out:

Patched:

  • Only blackness unless removing camera restrictions
  • Otherwise just right:

Perhaps next, the terrain tiles that are in the shroud of darkness could become transparent?

On the 0.3 change, I think that's just empirical from me.

Introduced in rP15576.

source/renderer/SkyManager.cpp
244 ↗(On Diff #7010)

"We" are the reader. It would be better to clarify that in the commit

253 ↗(On Diff #7010)

Where does this statement come from?

This revision was automatically updated to reflect the committed changes.