-
Notifications
You must be signed in to change notification settings - Fork 29
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
Advanced eclipses #395
base: main
Are you sure you want to change the base?
Advanced eclipses #395
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, I only found minor code style issues,
// Computes the refractive index of a gas at a given wavelength (in m), pressure (in Pa), and | ||
// temperature (in K). | ||
double getIoR(GasType type, double lambda, double pressure, double temperature) { | ||
double mu2 = std::pow(lambda * 1e6, -2.0); | ||
double n0 = 1.0; | ||
|
||
switch (type) { | ||
// E. R. Peck and B. N. Khanna. Dispersion of nitrogen | ||
case GasType::eNitrogen: | ||
n0 = 1.0 + 6.8552e-5 + 3.243157e-2 / (144 - mu2); | ||
break; | ||
|
||
// J. Zhang, Z. H. Lu, and L. J. Wang. Precision refractive index measurements of air, N2, O2, | ||
// Ar, and CO2 with a frequency comb. This is given at 20°C. | ||
case GasType::eOxygen: | ||
n0 = 1.0 + 1.181494e-4 + 9.708931e-3 / (75.4 - mu2); | ||
n0 = 1.0 + (n0 - 1) * 293.15 / 273; // Correct for the actual temperature. | ||
break; | ||
|
||
// E. R. Peck and D. J. Fisher. Dispersion of argon | ||
case GasType::eArgon: | ||
n0 = 1.0 + 6.7867e-5 + 3.0182943e-2 / (144 - mu2); | ||
break; | ||
|
||
// J. G. Old, K. L. Gentili, and E. R. Peck. Dispersion of Carbon Dioxide | ||
case GasType::eCarbonDioxide: | ||
n0 = 1.0 + 0.00000154489 / (0.0584738 - mu2) + 0.083091927 / (210.9241 - mu2) + | ||
0.0028764190 / (60.122959 - mu2); | ||
break; | ||
} | ||
|
||
// The pressure is given in Pa, the temperature in K. The IoR above is measured at STP (273 K, | ||
// 101325 Pa). We need to correct for the actual pressure and temperature. | ||
return 1.0 + (n0 - 1.0) * pressure / temperature * 273 / 101325.0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be more future proof, to read in the parameters for each gas from config files? This would make adding new gases easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess we could do this. But this code is currently not strictly required anyways, as we do not support wavelength-dependent refractive indices. It's just quite cool for future reference, I think.
bool RefractionSupported() { | ||
#if USE_REFRACTION | ||
return true; | ||
#else | ||
return false; | ||
#endif | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a function instead of a constant? A function call is more overhead (although the compiler will most likely optimize it away).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the model shader is linked to the main atmosphere shader which needs to know whether the model supports refraction. I think I cannot read a constant from a linked shader, or can I?
|
||
//////////////////////////////////////////////////////////////////////////////////////////////////// | ||
|
||
std::tuple<GLuint, int32_t, int32_t> read2DTexture(std::string const& path) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a tuple is not a good return type here. Since it is not clear to the caller, what each element is representing. The documentation in the header file also doesn't explain this. Only the return in the implementation can give a glimpse.
I think a better return type would either be a std::pair<GLuint, ivec2>
or a custom type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
//////////////////////////////////////////////////////////////////////////////////////////////////// | ||
|
||
std::tuple<GLuint, int32_t, int32_t, int32_t> read3DTexture(std::string const& path) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
std::tuple<GLuint, int32_t, int32_t> read2DTexture(std::string const& path); | ||
std::tuple<GLuint, int32_t, int32_t, int32_t> read3DTexture(std::string const& path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See CPP file for comment on return types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
const float D = 0.20; | ||
const float E = 0.02; | ||
const float F = 0.30; | ||
return ((c * (A * c + C * B) + D * E) / (c * (A * c + B) + D * F)) - E / F; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is hard to read with having an upper and lower case c in the same equation. Maybe use UVWXYZ or write color instead of c.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Co-authored-by: Jonas Gilg <[email protected]>
This PR brings two major contributions: