-
Notifications
You must be signed in to change notification settings - Fork 78
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
Avkhokhlov/light for stanalone #198
Avkhokhlov/light for stanalone #198
Conversation
|
||
namespace | ||
{ | ||
void LoadMateirals(std::string const& basepath, Scene1::Ptr scene) |
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.
Materials, not Mateirals
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
else if (type == "ibl") | ||
{ | ||
auto image_io = ImageIo::CreateImageIo(); | ||
auto tex = image_io->LoadImage(elem->Attribute("tex")); |
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.
Can we support paths relative to the light_file 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.
No, we can't
} | ||
else | ||
{ | ||
throw std::runtime_error("Unknown light 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.
What type exactly?
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
} | ||
|
||
tinyxml2::XMLElement* elem = root->FirstChildElement("light"); | ||
Light::Ptr light_instance; |
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.
Unused?
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.
Yep
Scene1::Ptr LoadScene(Baikal::AppSettings const& settings) | ||
{ | ||
#ifdef WIN32 | ||
std::string basepath = settings.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.
Here we should, probably, check if settings.path has a trailing slash.
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.
Probably
|
||
if (scene == nullptr) | ||
{ | ||
throw std::runtime_error("LoadScene(...): can not create scene"); |
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.
cannot
@@ -14,7 +14,9 @@ set(APPLICATION_SOURCES | |||
Application/graph_scheme.h | |||
Application/graph_scheme.cpp | |||
Application/material_explorer.h | |||
Application/material_explorer.cpp) | |||
Application/material_explorer.cpp | |||
Application/scene_load_utils.h |
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.
scene_loader?
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.
Nope
#include <memory> | ||
#include "app_utils.h" | ||
|
||
class Baikal::Scene1; |
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.
namespace Baikal {
class Scene1;
}
There is a corresponding CI build error.
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
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.
Please fix minor remarks and merge
{ | ||
if (light_file.empty()) | ||
{ | ||
return; |
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.
User probably does not intend to pass an empty light file, should that throw an exception?
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.
Nope, it's valid case, right there is no light file at all
throw std::runtime_error("Failed to open lights set file."); | ||
} | ||
|
||
tinyxml2::XMLElement* elem = root->FirstChildElement("light"); |
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.
auto?
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.
Yep
No description provided.