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.

Sign up for GitHub

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

Jump to bottom

Bug fix: add "reshape" functionality to utility viewer #322

Merged
merged 3 commits into from
Oct 31, 2019
Merged

Bug fix: add "reshape" functionality to utility viewer #322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d945f2
Bug fix: add "reshape" functionality to utility viewer
bigbike Oct 29, 2019
ad2caa3
minor
bigbike Oct 30, 2019
afb0c0a
minor
bigbike Oct 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/esp/gfx/RenderCamera.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ void RenderCamera::setProjectionMatrix(int width,
float zfar,
float hfov) {
const float aspectRatio = static_cast<float>(width) / height;
// TODO:
// Warning: By dedault, SceneGraph::Camera class can handle aspect ratio
// preservation in the setViewport() call automatically. But here it was
// disabled for some reason. Will revisit.
camera_->setAspectRatioPolicy(SceneGraph::AspectRatioPolicy::NotPreserved)
.setProjectionMatrix(
Matrix4::perspectiveProjection(Deg{hfov}, aspectRatio, znear, zfar))
Expand Down
13 changes: 9 additions & 4 deletions src/utils/viewer/viewer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ class Viewer : public Magnum::Platform::Application {

bool drawObjectBBs = false;

const float hfov_ = 90.0f;
const float znear_ = 0.01f;
const float zfar_ = 1000.0f;

Magnum::Timeline timeline_;
};

Expand Down Expand Up @@ -176,12 +180,9 @@ Viewer::Viewer(const Arguments& arguments)
rgbSensorNode_->translate({0.0f, rgbSensorHeight, 0.0f});
agentBodyNode_->translate({0.0f, 0.0f, 5.0f});

float hfov = 90.0f;
int width = viewportSize[0];
int height = viewportSize[1];
float znear = 0.01f;
float zfar = 1000.0f;
renderCamera_->setProjectionMatrix(width, height, znear, zfar, hfov);
renderCamera_->setProjectionMatrix(width, height, znear_, zfar_, hfov_);

// Load navmesh if available
const std::string navmeshFilename = io::changeExtension(file, ".navmesh");
Expand Down Expand Up @@ -359,6 +360,10 @@ void Viewer::drawEvent() {
void Viewer::viewportEvent(ViewportEvent& event) {
GL::defaultFramebuffer.setViewport({{}, framebufferSize()});
renderCamera_->getMagnumCamera().setViewport(event.windowSize());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SceneGraph::Camera class can handle aspect ratio preservation in the setViewport() call automatically -- you only need to call Camera::setAspectRatioPolicy() with a desired value.

Looking at the code, here it's set to NotPreserved for some reason, which effectively disables such handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Good to know. I do not know the purpose either. Will mark it, and solve it in recent "sensor/camera redesign" project (See slack for more details. Hope you can participate in this project. :-) )

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I commented here because doing it via setAspectRatioPolicy() would result in less code overall. Merging this as-is just piled up more unnecessary tech debt in my opinion.

NotPreserved is the default, and it probably makes sense for rendering headless -- there you have no arbitrary framebuffer resizes anyway. But having that there doesn't prevent the viewer from redefining it to handle this differently when having a GUI.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree with @mosra . There is no reason to keep the NotPreserved here and add extra code to implement behavior we could achieve by just changing the flag. Let's have a followup PR to remove the extra code + extra comments, and just switch the flag instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking when @msavva wrote it, he must have some reasons, so I was trying to respect that by not touching it, and leaving it to the future discussion.
But since he also leans towards changing it now, I am happy to write a new PR to roll back this one, and fix the issue in the other way.


int width = event.windowSize()[0];
int height = event.windowSize()[1];
renderCamera_->setProjectionMatrix(width, height, znear_, zfar_, hfov_);
}

void Viewer::mousePressEvent(MouseEvent& event) {
Expand Down