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.

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

added ability to transform points to screenspace when window resizes #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TheMaverickProgrammer
Copy link
Contributor

The window resizes but the mouse position returns values that is unusable for proper tracking. I added variables to track the initial window size and then provided a utility function to transform the input position to proper screen-space values that can be used after resizing.

I updated the framebuffer example to showcase this feature

Copy link
Owner

@ArthurSonzogni ArthurSonzogni left a comment

Choose a reason for hiding this comment

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

Thank you!

I let some comments below:

@@ -107,6 +107,11 @@ void GLFWCharCallback(GLFWwindow* glfw_window, unsigned int codepoint) {

} // namespace

glm::vec2 smk::Window::ToScreenSpace(const glm::vec2& world) {
glm::vec2 factor = glm::vec2(initialSize_.x/width_, initialSize_.y/height_);
return glm::vec2(world.x*factor.x, world.y*factor.y);
Copy link
Owner

Choose a reason for hiding this comment

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

The view can also be rotated / skewed.

void RenderTarget::SetView(const View& view) {

  • I think it would be better to use the RenderTarget::project_matrix for this.
  • I don't think we need to keep track of the initial size.
  • I think this can be moved to RenderTarget.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm yeah the view could be accounted for. That would make sense and become very useful. My original intention was to match the window size even after resizing. I originally had this event handled by Input and used the resize event for glfw to feed it, but I thought maybe it was more appropriate for the window.

@@ -107,6 +107,11 @@ void GLFWCharCallback(GLFWwindow* glfw_window, unsigned int codepoint) {

} // namespace

glm::vec2 smk::Window::ToScreenSpace(const glm::vec2& world) {
Copy link
Owner

Choose a reason for hiding this comment

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

The name is a bit confusing, I would have thought we went from screen space to world coordinate.
What about MapPixelToWorldCoordinates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the input is the world position and the output is the screenspace. This is pretty standard convention. Unsure how to make this more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MapPixelToWorldCoordinates would be appropriate the more I think about it. SFML uses this name too for that purpose I assume

@TheMaverickProgrammer
Copy link
Contributor Author

TheMaverickProgrammer commented Oct 17, 2020

I changed the name of the function to MapPixelToCoords

Ideally this function should go into RenderTarget and use the View to project the point into the render target's view-space. This will also allow things like rotations to be mapped correctly.

Regardless if you accept or reject this merge, I am upgrading the View class to use an internal projection matrix like SFML does in order to achieve the aforementioned upgrades. You can see my progress here: https://github.com/TheMaverickProgrammer/smk/tree/feature/UpgradedViewStruct

Unfortunately my math is wrong somewhere because the projection matrix ends up skewing the screen diagonally. (I even changed where I needed to pass in the projection matrix at the shader level but reverted it since clearly something is wrong somewhere).

@ArthurSonzogni
Copy link
Owner

Ideally this function should go into RenderTarget and use the View to project the point into the render target's view-space. This will also allow things like rotations to be mapped correctly.

I totally agree.

Regardless if you accept or reject this merge, I am upgrading the View class to use an internal projection matrix like SFML does in order to achieve the aforementioned upgrades. You can see my progress here: https://github.com/TheMaverickProgrammer/smk/tree/feature/UpgradedViewStruct

This sounds like a good thing to do. I don't fully remember why I didn't used a glm::mat4 internally. Maybe I wanted to provide accessors like Width() and Height().

If you are facing some problems and think you can't make any progress anymore, feel free to tell me and I will try fixing the remaining problems.

@TheMaverickProgrammer
Copy link
Contributor Author

I have halted working on it to finish the coding style corrections. Feel free to pull the branch and poke around. I was close. Once I'm done with coding style fixes I'll switch to that.

@TheMaverickProgrammer
Copy link
Contributor Author

The end of 2020 was not kind to me and delayed me. This is just an update that I will return to this project early this year. 👍

@TheMaverickProgrammer
Copy link
Contributor Author

Update: early did not happen. But I will return because I want to port my game engine to smk:
https://twitter.com/OpenNetBattle

@ArthurSonzogni
Copy link
Owner

Update: early did not happen. But I will return because I want to port my game engine to smk:
https://twitter.com/OpenNetBattle

;-) I never doubted you would come back!

The latest video of the looks really nice! A great attention to detail.

Happy to help you if there are any missing features in SMK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants