Read and use resolution.xml by maaxxaam · Pull Request #32 · OpenAWE-Project/OpenAWE

maaxxaam · 2023-05-28T22:01:52Z

This PR's goal is to make OpenAWE read resolution.xml, which it currently only writes zeroes to, and use it to actually resize the window and make it fullscreen if specified. Also adds defaults for resolution.xml based of user's current video mode if file is absent.

This is a draft PR now because fullscreen scaling for lower resolutions is wrong (doesn't stretch to the full screen) and needs fixing before being useful. Windowed mode works fine:

Screenshot: OpenAWE running at 1280x720

Note: check if you have pre-existing resolution.xml before running. Most likely, it's filled with zeroes - change those to the desired resolution, or OpenAWE will crash.

Nostritius

I have added some comments with some things that sticked to my eye. I will will make a full review, when you request it.
I am currently working on a big update on the renderer, where the screen size is important (It's about lightning in the scene). Do you want me to push this first or wait for your PR?

src/common/platform.cpp

src/graphics/renderer.h

src/graphics/opengl/renderer.cpp

src/platform/context.h

maaxxaam · 2023-05-30T09:54:37Z

Thank you for your feedback. I hope to get to it soon enough.

I am currently working on a big update <..>. Do you want me to push this first or wait for your PR?

You should definitely go along with your plans. Unfortunately, I still don't have much time to work on this PR for personal reasons, so this might take a while.

maaxxaam · 2023-09-12T19:17:28Z

Alright, looks like this PR was able to withstand a rebase with a couple of merges. Good to know that's possible.

I managed to get down to the reason why fullscreen didn't scale properly for me: as it turns out, as of GLFW 3.3.8 fullscreen apps in Wayland use a system-wide resolution no matter what, meaning that calling glfwSetWindowSize() and alike while the window is in fullscreen mode does nothing. I've tried to remedy this by introducing scaling, but ran into some weird behavior: while GUI does seem to scale correctly, the world itself still tries to render with as much quality as possible.

Edit: here's an example of the behavior I'm talking about:

Screenshot

Running OpenAWE on Wayland. `resolution.xml` is set to 640x360 fullscreen while the screen resolution is 1920x1080.

Also, I forgot to mention in the commit message that now configuration code treats 0x0 resolution (usually left by latest commits in the config) as unknown, which should be a touch better handling.

Nostritius

I have tested the code in a wayland environment and noticed the same problem. Stupidly it seems wayland does not want to support modesetting (Changing the resolution of the screen). Maybe you can even check if wayland is used by the $XDG_SESSION_TYPE environment variable. Take into account that this is also a problem in Xwayland. Another option would be to just force the user to the screens resolution and print a warning about it. But I admit that having true modesetting available in wayland is a plus. It can take pressure from the gpu as I noticed myself with a 4k screen and a rather old nvidia gpu. It might be possible over changing the screens resolution in the same way as the settings app does?. Just my thoughts on the matter.

Nostritius · 2023-09-18T20:41:20Z

src/common/platform.cpp

 #include <locale>
 #include <regex>

+#include <GLFW/glfw3.h>


This include should not appear outside the platform library

Nostritius · 2023-09-18T20:41:57Z

src/common/platform.cpp

+#include <GLFW/glfw3.h>
+
 #include "src/common/platform.h"
+#include "src/common/exception.h"


I think this was only used for the monitor function, so it could be removed as well

Nostritius · 2023-09-24T09:11:15Z

src/graphics/opengl/renderer.h

 	void drawFrame() override;

+	void setRenderPlane(unsigned int width, unsigned int height) override;
+	void setRenderPlane(glm::vec2 size) override;


Please rename also these methods to setRenderPlane

Nostritius · 2023-09-24T09:13:11Z

src/platform/window.cpp

+void Window::setFullscreen(bool fullscreen) {
+	unsigned int width, height;
+	getSize(width, height);
+	glfwSetWindowMonitor(_window, fullscreen ? glfwGetPrimaryMonitor() : NULL, 0, 0, width, height, GLFW_DONT_CARE);


Please use nullptr instead of NULL for uniformity

Nostritius · 2023-09-24T12:47:07Z

src/graphics/opengl/framebuffer.cpp

 * along with OpenAWE. If not, see <http://www.gnu.org/licenses/>.
 */

+#include <algorithm>


Do you need this include?

Nostritius · 2023-09-24T12:47:35Z

src/graphics/opengl/framebuffer.cpp

+	const GLenum framebufferStatus = glCheckFramebufferStatus(GL_FRAMEBUFFER);
+
+	if (framebufferStatus != GL_FRAMEBUFFER_COMPLETE)
+		throw CreateException("Failed to initialize framebuffer: {}", framebufferStatus);


Is there a reason why you split up the framebuffer status?

It was not specifically splitting, I was just outputting the error code in the exception message. I'll revert this change - it doesn't have to be here.

Nostritius · 2023-09-24T12:48:09Z

src/graphics/opengl/renderer.cpp

+    assert(glGetError() == GL_NO_ERROR);
+}
+
+void Renderer::setRenderPlane(unsigned int width, unsigned int height) {


Change this to SetViewportSize

Nostritius · 2023-09-24T12:48:46Z

src/graphics/opengl/renderer.cpp

+	setRenderPlane(glm::vec2(width, height));
+}
+
+void Renderer::setRenderPlane(const glm::vec2 size) {


Change this to SetViewportSize

Nostritius · 2023-09-24T12:51:52Z

src/platform/context.h

 class Context {
 public:
 	virtual void getSize(unsigned int &width, unsigned int &height) = 0;
+	virtual glm::vec2 getSize() = 0;


Do we need this method? I would prefer having the context interface to be designed in a minimal and concise way and if necessary convert it somewhere else to float

maaxxaam · 2023-10-12T16:23:40Z

src/game.cpp

+	_window->setFullscreen(config.resolution.fullscreen);
+	_window->setSize(config.resolution.width, config.resolution.height);
+	spdlog::error("Window size: {} x {}", _window->getSize().x, _window->getSize().y);
+	spdlog::error("Content scale: {} x {}", _window->getContentScale().x, _window->getContentScale().y);


Note to self - remove these log messages as well

Also writes resolution.xml based of user's current video mode if absent Note: fullscreen scaling for lower resolutions is still wrong, needs fixing

- Add content scaling (because of how Wayland does fullscreen) - Rename _renderPlane to _viewportSize - Move getPrimaryMonitorVideoMode to Platform - Remove setSize/Fullscreen from Context

maaxxaam force-pushed the resolution-setter branch from e7dc8f4 to c977a2b Compare May 28, 2023 22:05

Nostritius reviewed May 29, 2023

View reviewed changes

src/common/platform.cpp Outdated Show resolved Hide resolved

src/graphics/renderer.h Outdated Show resolved Hide resolved

src/graphics/opengl/renderer.cpp Outdated Show resolved Hide resolved

src/platform/context.h Outdated Show resolved Hide resolved

maaxxaam force-pushed the resolution-setter branch from c977a2b to b7b99e3 Compare September 12, 2023 19:07

Nostritius requested changes Sep 24, 2023

View reviewed changes

maaxxaam commented Oct 12, 2023

View reviewed changes

maaxxaam added 3 commits July 10, 2024 17:28

Read resolution.xml config on startup

7c8e88a

Also writes resolution.xml based of user's current video mode if absent Note: fullscreen scaling for lower resolutions is still wrong, needs fixing

Fullscreen video mode patch attempt

b3c198f

More resolution fixes

7885ff9

- Add content scaling (because of how Wayland does fullscreen) - Rename _renderPlane to _viewportSize - Move getPrimaryMonitorVideoMode to Platform - Remove setSize/Fullscreen from Context

maaxxaam force-pushed the resolution-setter branch from b7b99e3 to 7885ff9 Compare July 11, 2024 16:19

Conversation

maaxxaam commented May 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nostritius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maaxxaam commented May 30, 2023

Uh oh!

maaxxaam commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nostritius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maaxxaam commented May 28, 2023 •

edited

Loading

maaxxaam commented Sep 12, 2023 •

edited

Loading