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

Add option to rotate outputs #137

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Conversation

axtloss
Copy link
Contributor

@axtloss axtloss commented Jan 28, 2024

Adds support for output Transformation using a config option transform in the output section to allow setting it per output
closes #106

Attempting to do a screenshot on a rotated output (tested with rotation 270) currently cuts the available output to screenshot by half, any part of the selected area that goes into the "invisible" area becomes transparent

@axtloss axtloss marked this pull request as draft January 28, 2024 13:31
Copy link
Owner

@YaLTeR YaLTeR left a comment

Choose a reason for hiding this comment

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

Thanks! Some more things:

  1. Screenshot should work fine. Changing transform when the screenshot UI is open shouldn't break it. Currently if the scale changes the screenshot UI will close automatically, just do the same with transform.
  2. Screencast should also work fine. Things should not break when the monitor is rotated while it's being screencasted. For now I suggest just stopping the screencast if that happens, because if the monitor is rotated 90° then the buffer dimensions would change, which there's no code to handle atm.
  3. Niri::refresh_pointer_outputs() seems to hardcode Transform::Normal for the clients right now, it should use the current output transform.

@axtloss
Copy link
Contributor Author

axtloss commented Jan 28, 2024

(i will apply the code suggestions in a bit)

Screenshot should work fine. Changing transform when the screenshot UI is open shouldn't break it. Currently if the scale changes the screenshot UI will close automatically, just do the same with transform.

Weird, for me part of the transformed display just disappears in some way, the resulting screenshot turns transparent:
image

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 28, 2024

You probably need to change some hardcoded Transform values inside screenshot UI.

@axtloss
Copy link
Contributor Author

axtloss commented Jan 28, 2024

I tested with obs, and I think I see what happens, despite the output being transformed what screen captures see are an untransformed screen
image

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 28, 2024

Yeah, I think also might be caused by some hardcoded Transform values.

@axtloss
Copy link
Contributor Author

axtloss commented Jan 28, 2024

Yeah, I think also might be caused by some hardcoded Transform values.

Yes that appears to be the issue, changing some hardcoded values around changes makes it see the output correctly.

@cmeissl
Copy link
Contributor

cmeissl commented Jan 28, 2024

This should at least document that the transform is counter clock wise as defined by the wayland protocol spec.
Otherwise this could create some confusion especially with automated changes based on hardware sensors which
might put out clock wise rotation. An alternative would be to name it orientation, define it as CW and invert before applying to the wayland outpu.

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 28, 2024

Hmm. Sway calls it "transform" and performs it clockwise. I think we should do it clockwise too?

@cmeissl
Copy link
Contributor

cmeissl commented Jan 28, 2024

Hmm. Sway calls it "transform" and performs it clockwise. I think we should do it clockwise too?

Also fine, as long as it is documented/specified somewhere.
This will still require to invert the transform before setting it on the Output, because this transform is
defined as CCW.

@axtloss
Copy link
Contributor Author

axtloss commented Jan 28, 2024

The latest commit fixes the screenshot issue somewhat, the preview ignores the transform and is rotated normally, causing part of the desktop to be cut off, however the actual screenshot functions fine.
I am not sure how to fix the rendering issue, I would greatly appreciate some help/indicators where the issue could be

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 28, 2024

Maybe due to the hardcoded transform in render_to_texture()? Might need to set it there and hardcode Normal back in screenshot UI; if you don't manage to get it working I'll take a look myself.

@axtloss
Copy link
Contributor Author

axtloss commented Jan 28, 2024

This should at least document that the transform is counter clock wise as defined by the wayland protocol spec. Otherwise this could create some confusion especially with automated changes based on hardware sensors which might put out clock wise rotation. An alternative would be to name it orientation, define it as CW and invert before applying to the wayland outpu.

I made it rotate clockwise and also mentioned it in the default config

@axtloss
Copy link
Contributor Author

axtloss commented Jan 28, 2024

Maybe due to the hardcoded transform in render_to_texture()? Might need to set it there and hardcode Normal back in screenshot UI; if you don't manage to get it working I'll take a look myself.

I made render_to_texture() use the current transform, but it still does not work, it now is upside down and still cut off. I am not really sure what else to try

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 28, 2024

I'll take a look myself then (but not today).

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 29, 2024

Figured it out. Gonna push some commits into this PR.

@axtloss
Copy link
Contributor Author

axtloss commented Jan 29, 2024

that's nice! should I invite you as a collaborator on my fork to make it easier to add the changes?

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 29, 2024

It says "Maintainers are allowed to edit this pull request." which means I should be able to push to this branch.

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 29, 2024

Pushed some fixes, please try if everything works. I changed transform to mean counter-clockwise (think it's more important to avoid confusion between the types).

There are still a few bugs left, notably with direct scanout and with changing transform on the fly, but I suspect those need Smithay fixes.

@axtloss
Copy link
Contributor Author

axtloss commented Jan 29, 2024

What bugs did you experience with changing transform on the fly? When I tested with a nested niri session it worked fine

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 29, 2024

  • if you get direct scanout with 90 or 270 transform, it will show up as the inverse transform instead
  • if you change transform on the fly in such a way that width and height aren't swapped (e.g. normal to 180 or normal to flipped or 90 to 270) then the monitor isn't damaged, so you'll see artifacts until the monitor is eventually fully redamaged again

@axtloss
Copy link
Contributor Author

axtloss commented Jan 29, 2024

Is it possible to damage an output or force a redraw on it? I tried using niri.redraw() but that just crashes niri so I probably did something wrong

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 29, 2024

I think we can fix this in Smithay instead

@axtloss
Copy link
Contributor Author

axtloss commented Jan 29, 2024

Actually it turns out that the crash weren't caused by my changes but changing the transform on the fly generally just appears to cause niri to crash now

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 29, 2024

Hmm? I haven't seen any crashes. Could you post the backtrace please?

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 29, 2024

@cmeissl fixed both issues I mentioned in #143 (and the corresponding Smithay PR). After that lands this PR should be good to go, barring the crashes that you found, but I need a backtrace since I haven't managed to crash this yet.

@axtloss
Copy link
Contributor Author

axtloss commented Jan 29, 2024

The crashes stopped happening and no matter what I do I cant seem to get them back, maybe it was fixed by me rebooting the computer

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 30, 2024

Another win for the turning it off and on again computer fixing strategy

Have you by chance been running niri as a session when you got the crashes? Then they might still be present in journalctl -b -1 /usr/bin/niri

@YaLTeR YaLTeR force-pushed the feat/support-rotation branch from cb369da to 401e4eb Compare January 30, 2024 11:38
@YaLTeR
Copy link
Owner

YaLTeR commented Jan 30, 2024

I updated to the latest Smithay, and something I just noticed that might be new after the update: "flipped-90" and "flipped-270" are both vertically and horizontally flipped relative to their normal counterparts, whereas "flipped" and "flipped-180" are only horizontally flipped. In sway each flip is only horizontally flipped. @cmeissl could this be something that has regressed?

@cmeissl
Copy link
Contributor

cmeissl commented Jan 30, 2024

I updated to the latest Smithay, and something I just noticed that might be new after the update: "flipped-90" and "flipped-270" are both vertically and horizontally flipped relative to their normal counterparts, whereas "flipped" and "flipped-180" are only horizontally flipped. In sway each flip is only horizontally flipped. @cmeissl could this be something that has regressed?

Unlikely that this is a regression from the latest changes, can you give me simple reproduction steps?

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 30, 2024

Set monitor transform "90", then change to "flipped-90", observe that it's flipped in both ways (so it should be "flipped-270" instead).

@cmeissl
Copy link
Contributor

cmeissl commented Jan 30, 2024

Set monitor transform "90", then change to "flipped-90", observe that it's flipped in both ways (so it should be "flipped-270" instead).

Yeah, can reproduce this and no, this can not be caused by this changes.
I will take a look a this later this evening.

@axtloss
Copy link
Contributor Author

axtloss commented Jan 30, 2024

Another win for the turning it off and on again computer fixing strategy

Have you by chance been running niri as a session when you got the crashes? Then they might still be present in journalctl -b -1 /usr/bin/niri

Yep theres a crash log in journalctl, though I still cannot reproduce it

thread 'main' panicked at src/niri.rs:1994:9:
assertion failed: matches!(state.redraw_state, RedrawState :: Queued(_) | RedrawState ::
    WaitingForEstimatedVBlankAndQueued(_))
stack backtrace:
   0: rust_begin_unwind
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/panicking.rs:72:14
   2: core::panicking::panic
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/panicking.rs:127:5
   3: niri::niri::Niri::redraw
             at ./Projects/niri/src/niri.rs:1994:9
   4: niri::niri::Niri::queue_redraw::{{closure}}
             at ./Projects/niri/src/niri.rs:1631:13
   5: calloop::loop_logic::LoopHandle<Data>::insert_idle::{{closure}}
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.12.4/src/loop_logic.rs:156:17
   6: <core::option::Option<F> as calloop::sources::IdleDispatcher<Data>>::dispatch
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.12.4/src/sources/mod.rs:597:13
   7: calloop::loop_logic::EventLoop<Data>::dispatch_idles
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.12.4/src/loop_logic.rs:542:13
   8: calloop::loop_logic::EventLoop<Data>::dispatch
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.12.4/src/loop_logic.rs:560:9
   9: calloop::loop_logic::EventLoop<Data>::run
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/calloop-0.12.4/src/loop_logic.rs:596:13
  10: niri::main
             at ./Projects/niri/src/main.rs:249:5
  11: core::ops::function::FnOnce::call_once
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
2024-01-29T18:10:15.297033Z DEBUG niri::watcher: exiting watcher thread for /home/xen/.config/niri/config.kdl

@axtloss
Copy link
Contributor Author

axtloss commented Jan 30, 2024

I updated to the latest Smithay, and something I just noticed that might be new after the update: "flipped-90" and "flipped-270" are both vertically and horizontally flipped relative to their normal counterparts, whereas "flipped" and "flipped-180" are only horizontally flipped. In sway each flip is only horizontally flipped. @ cmeissl could this be something that has regressed?

I am able to reproduce this on a build that does not have the updated smithay yet

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 30, 2024

Oh wow, thanks for that log. That is a part that is definitely not supposed to crash like that.

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 30, 2024

Do you happen to remember what you did right before you got that crash?

@axtloss
Copy link
Contributor Author

axtloss commented Jan 30, 2024

I edited the configuration to change the transform value for a screen, if it matters, the open application were firefox, foot and emacs

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 30, 2024

I'm really not sure how that could've happened but I doubt the new transform code has anything to do with it, so I won't block this PR on that crash. However, I'll wait a bit more so that we can investigate the flipped-90/270 thing, and I will also once again check other compositors to settle on the clockwise vs. counter-clockwise.

@cmeissl
Copy link
Contributor

cmeissl commented Jan 31, 2024

I'm really not sure how that could've happened but I doubt the new transform code has anything to do with it, so I won't block this PR on that crash. However, I'll wait a bit more so that we can investigate the flipped-90/270 thing, and I will also once again check other compositors to settle on the clockwise vs. counter-clockwise.

Can you give Smithay/smithay#1311 a try?

@YaLTeR YaLTeR marked this pull request as ready for review January 31, 2024 18:34
@YaLTeR
Copy link
Owner

YaLTeR commented Jan 31, 2024

Alright, now with the extra fix from @cmeissl everything seems to work fine.

@YaLTeR
Copy link
Owner

YaLTeR commented Jan 31, 2024

Rotation direction wise:

  • sway is clockwise by default
  • hyprland is counter-clockwise by default

So I guess if there's no "standard" we can stick with counter-clockwise, as that makes the types match.

@YaLTeR YaLTeR force-pushed the feat/support-rotation branch from a810ee3 to 9bb37e8 Compare January 31, 2024 18:59
@YaLTeR YaLTeR merged commit 2e50f8d into YaLTeR:main Jan 31, 2024
5 checks passed
@YaLTeR
Copy link
Owner

YaLTeR commented Jan 31, 2024

Thanks @axtloss and @cmeissl!

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.

Add monitor rotation setting
3 participants