Page MenuHomeWildfire Games

Removes hardcoded SkyBox sizes and use infinity sky
Needs ReviewPublic

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

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
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
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6472
Build 10715: Vulcan BuildJenkins

Event Timeline

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

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

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

Angen awarded a token.Thu, Nov 29, 7:19 AM
smiley added a subscriber: smiley.Thu, Nov 29, 7:56 AM
smiley added inline comments.
source/renderer/SkyManager.cpp
119

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

vladislavbelov marked an inline comment as done.Thu, Nov 29, 8:19 AM
vladislavbelov added inline comments.
source/renderer/SkyManager.cpp
119

Do you mean hardcoded extension?

smiley added inline comments.Thu, Nov 29, 8:24 AM
source/renderer/SkyManager.cpp
119

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

vladislavbelov marked an inline comment as done.Thu, Nov 29, 8:42 AM
vladislavbelov added inline comments.
source/renderer/SkyManager.cpp
119

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

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

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

elexis updated the Trac tickets for this revision.Thu, Nov 29, 11:15 AM
vladislavbelov marked 2 inline comments as done.Thu, Nov 29, 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

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

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