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

Dynamic screen size #39

Merged
merged 5 commits into from
Oct 2, 2021
Merged

Conversation

jlloh
Copy link
Contributor

@jlloh jlloh commented Sep 27, 2021

Attempts to address #29

Use OS.get_screen_size to dynamically set resolution. Assumes full screen used.

Also a minor fix to center the window, using OS.set_window_position.

Tested on: Manjaro with Gnome

Use `OS.get_screen_size` to dynamically set resolution. Assumes full screen.

Also a minor fix to center the window, using `OS.set_window_position`.
@AsherGlick
Copy link
Owner

Thanks for the PR. This change seems to breaking for both multi monitor setups where the game is not on the leftmost monitor, and setups where the game is in windowed mode. Though windowed mode did not work 100% before I would like to avoid pulling code that breaks it more.

Ideally these values would be gathered from the gw2 window, maybe using burrito-fg and calling breadx's Window::geometry_immediate()

But in lieu of proper automation these values should be user configurable, such as an input box for width and height. In the menu.

@jlloh
Copy link
Contributor Author

jlloh commented Sep 28, 2021

Hmm so I would need to somehow track the position and size of the current GW2 window, and "track it" and position it. That's probably preferable to manual user inputs i would think. Let me give it a go 👍

@coderedart
Copy link
Contributor

coderedart commented Sep 28, 2021

If the position from get geometry function is wrong, it's because x11 sends the position relative to the parent window which might be another invisible window or the frame window.

After getting the geometry, you have to translate the coordinates relative to root which you get with geometry.

breadx::auto::xproto::TranslateCoordinatesRequest

relevant stack overflow answer:
https://stackoverflow.com/a/50080357/7484780

For reference with a different x11 library,
https://github.com/coderedart/jokolay/blob/c4f5d11af9de7842fc1c1b8b5d6022c17caa9f1b/src/core/window/linux.rs#L111

@AsherGlick
Copy link
Owner

Every PR for this repo I learn something new about X11.

@coderedart If windows can be nested in each other does it make sense to nest burrito inside of the guildwars window? Then just have its offset be 0? Or does this seems like an unintended usecase for the feature?

https://docs.rs/breadx/2.0.0/breadx/auto/xproto/struct.Window.html#method.reparent seems relevant but I have not looked past the documentation.

@coderedart
Copy link
Contributor

coderedart commented Sep 29, 2021

x11's children clip the parent window, EVEN when transparent. so, the part of the game window that burrito covers will be a black screen (usually we cover the whole gw2 window). so, we are basically back to running taco on linux via wine xD which has a black window too.

for reference: https://stackoverflow.com/a/31436071/7484780

I eventually learn about Xembed, but that won't work because it requires parent/child windows to communicate, which gw2 won't do as parent or child. yabridge, does that by going super low level with wine/x11 (in C/C++), but I don't think any overlay for a gw2 will be ever worth that much effort.

transient_for is enough for 90% of things. for kde atleast, windowed fullscreen will probably work, its kinda blocked by https://bugs.kde.org/show_bug.cgi?id=441074 and https://bugs.kde.org/show_bug.cgi?id=442720 . the next step is pointer grabbing and stacking order, if someone wants to play with that, which will help with windowed fullscreen.

@AsherGlick
Copy link
Owner

In that case I think parsing the offsets to get an offset to root makes sense. For this PR though I think just extracting the height and width is sufficient. That would fix the multi resolution issue, but would still leave the offset issue to be corrected at a later time.

@coderedart
Copy link
Contributor

coderedart commented Sep 29, 2021

users will have to drag the burrito window if we don't set the offset.
we also have to consider that we won't get xid until mumble is initialized. so, until then,maybe https://docs.godotengine.org/en/stable/classes/class_os.html#class-os-method-get-screen-size to set the resolution/size, and then set size to gw2 window when we get xid from link.

@jlloh
Copy link
Contributor Author

jlloh commented Sep 29, 2021

Not much luck getting the position from BreadX. The x and y of the Geometry object seems to stay fixed, and I even tried inspecting one level above but the root is the display. Can't seem to find any invisible windows or whatnot.

Anyway for now, I've settled for just getting the resolution of the GW2 window by retrieving the xid using get_window_from_name function that you guys previously wrote.

Let me know if this approach makes sense.

Spatial.gd Show resolved Hide resolved
#[export]
fn get_gw2_geometry(&self, _owner: &Node) -> (u16, u16){
let mut dpy = DisplayConnection::create(None, None).unwrap();
let window = get_window_from_name(&mut dpy, String::from("Guild Wars 2"));
Copy link
Contributor

Choose a reason for hiding this comment

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

this has the issue that if guild wars didn't start yet, overlay will crash.
get window from name had issues. It didn't work on my pc last time :( the little updater window and the login window before the char select screen, both have the same window id / window name, so overlay will resize to them prematurely.

you can use the x11_window_id_gw2 similar to the function that sets transient_for . this has the advantage, that we can be sure that gw2 has already started and is logged into character screen.

Copy link
Contributor

@coderedart coderedart Sep 29, 2021

Choose a reason for hiding this comment

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

I'm not saying that we shouldn't use id_from_window_name function because it didn't work on my pc. but because it was super hard to actually debug that kind of issue, so if other users get similar issues, debugging will be that much harder, compared to just using the xid that we get from mumble link reliably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah I did look at x11_window_id_gw2 but on my machine it did not seem to have the right x11 id when I printed it out. That's why I didn't use it.

But sure let me take a look again, I might be missing something obvious. Will see if I can modify the function to take a x11 window id, and pass that in.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with @coderedart that we should try to use the xid value passed in similar to how set_transient_for works. Something to note is that the xid and pid are different values entirely. I would be surprised to learn that wine is setting a non-null incorrect xid value.

Because Burrito does not really do anything until the first time it receives a message from burrito link I believe we can just hold off on resizing until after we receive that first message. This would avoid crashing due to not having elements of that message already sent.

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid starting at random sizes or too big like 1920x1080, maybe we can set to a default startup dimensions. I chose 800x600 as that seems to be a popular basic size. Then, when gw2 xid is available, burrito can resize to gw2 size.

FHD is very common, but due to taskbar (especially when it hides and comes back), burrito icon almost always is in a weird position. 800x600 can be more easier to see the icon on desktop .

Copy link
Owner

Choose a reason for hiding this comment

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

Seems reasonable to start smaller, then grow. We could even start with a small splash screen like window. I don't want to put the burden on this PR for that but we can open a new issue to design it further.

@jlloh
Copy link
Contributor Author

jlloh commented Sep 30, 2021

Passing in x11_window_id_gw2

func decode_context_packet(spb: StreamPeerBuffer):
	compass_width = spb.get_16()
	compass_height = spb.get_16()
	var old_map_id = self.map_id
	self.map_id  = spb.get_32()

	var x11_window_id_gw2 = spb.get_32()
	if !is_transient:
		is_transient = x11_fg.set_transient_for(x11_window_id_burrito, x11_window_id_gw2)
	var locationTuple = x11_fg.get_window_geometry(x11_window_id_gw2)
	#print("Location Tuple")

Results:
image

Inspecting the window:

~ >>> xdotool selectwindow getwindowpid getwindowgeometry getdisplaygeometry getwindowname         
2399
Window 58720263
  Position: 0,0 (screen: 0)
  Geometry: 2560x1440
2560 1440
Guild Wars 2

Somehow the x11_id doesn't match. Not sure if I'm missing something obvious.

@coderedart
Copy link
Contributor

can you try printing the gw2_id in transient_for function too? I just did it and my x11 id is correct. i used the same
xdotool selectwindow getwindowpid getwindowgeometry getdisplaygeometry getwindowname as you and printed the xid in transient function.

@jlloh
Copy link
Contributor Author

jlloh commented Sep 30, 2021

🤦‍♂️ Newbie mistake. I did not realise the burrito_link binary in the release was not up to date, and did not include 7667d9f.

After recompiling burrito_link from master, it works as expected.

@jlloh
Copy link
Contributor Author

jlloh commented Sep 30, 2021

Ok, tested, ready for review. As suggested, I start off with 800x600, and resize after burrito_link starts sending passing the x11_id along.

@jlloh jlloh requested a review from AsherGlick October 2, 2021 09:59
Copy link
Owner

@AsherGlick AsherGlick left a comment

Choose a reason for hiding this comment

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

Looks good to me

@phemmer
Copy link

phemmer commented Oct 3, 2021

This change makes the situation worse for me. Using the last release (or up to build ccd2fd0), I could use wmctrl to override the size. I'm not sure what's happening in this build (6d394a9), but a few seconds after launching, Burrito disappears entirely. It seems like maybe it's setting the size to 0x0, as I can use wmctrl to bring it back, but then a few seconds later it disappears again.

Not sure what info is relevant, but I'm using XFCE.

@AsherGlick
Copy link
Owner

@phemmer can you confirm that the size is being set to 0x0 for your given setup?

You can probably easily do this by adding the line

godot_print!(format!("{} {}", geometry.width, geometry.height));

right after

let geometry = window.geometry_immediate(&mut dpy).unwrap();

If so can you open a new issue to track this?

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.

4 participants