maximbaz / wluma

Automatic brightness adjustment based on screen contents and ALS
ISC License
641 stars 28 forks source link

Update ash & some other deps #118

Closed Rishik-Y closed 2 weeks ago

Rishik-Y commented 3 weeks ago

I updated the ash and other dependencies to the latest version (Except Wayland-client and Wayland-protocol).

Added anyhow (version = "1.0") to fix some of the issues which normally would require writing a new code.

I just tried my best not to let any errors occur, but that doesn't mean the code is functioning properly. As far as I tried, I don't seem to have faced an issue, but Please rewrite the code however you fit @maximbaz

Again Thanks for your help

Rishik-Y commented 3 weeks ago

Wait really sorry Forgot about cargo.lock

Rishik-Y commented 3 weeks ago

I am sure you may be getting notifications every time I change something here. (Including my mistakes) Again, I am really sorry for my mistake, but I have updated it successfully. Please check the code at your leisure time.

Rishik-Y commented 2 weeks ago

Hello, @maximbaz Just a quick question, When keeping wlroots as a compositor, will it start detecting a white background and a black background immediately? As soon as the program starts running,

Because after upgrading ash to v0.38 The program seems to work like before, except when I run it with wlroots, it won't show any error, but it doesn't seem to change anything when going from white to black background either.

I don't know whether I made a mistake where I forgot about considering the wlroots while changing the vulkan.rs code or something else occured.

maximbaz commented 2 weeks ago

Hello, I think it should react immediately, but to avoid any potential misconfiguration or setup issues, could you try with the main branch to see if vulkan+wlroots reacts there?

Rishik-Y commented 2 weeks ago

Hello @maximbaz, Thank you for your immediate reply. I just tested the Main branch as well as the one i updated ash to v0.38 And for some reason, both are now properly. (EDIT: Working properly as in, The error is being shown in both)

I have tried a couple of times, and it shows the same error thrown at me in the main branch wluma.

My assumption is that i was running both the wluma (AUR) as well as the cargo run which seems to mess up with wlroots at that moment.

Rishik-Y commented 2 weeks ago

Sorry to ask but I just saw https://github.com/maximbaz/wluma/pull/86#issuecomment-2320923641 and just curious, was his/her code working? If it is, I won't mind closing this pull request. Since I made it not knowing someone else had already fixed the issue.

maximbaz commented 2 weeks ago

Unfortunately I'm in a position where I actually can't test vulkan part as my current hardware does not support vulkan 😭 So I don't know if it actually works. In the interest of getting something merged, I would suggest you continue the PR, especially as you have the ability to run it and verify that it works, and compare with the main branch. I think it's totally worth it to compare your ideas with how they done it in their patch, if nothing else then just in the interest of learning, but especially if you get stuck on something.

Rishik-Y commented 2 weeks ago

Unfortunately I'm in a position where I actually can't test vulkan part as my current hardware does not support vulkan 😭 So I don't know if it actually works. In the interest of getting something merged, I would suggest you continue the PR, especially as you have the ability to run it and verify that it works, and compare with the main branch. I think it's totally worth it to compare your ideas with how they done it in their patch, if nothing else then just in the interest of learning, but especially if you get stuck on something.

Yeah, Absolutely! πŸ˜€ I'm eager to see Wluma work on Hyprland, but more than that, I am happy that I am able to learn rust indirectly from doing it.

Let's also use ~1.0 as other deps do - by the way, love the idea of trying this crate out πŸ‘

In a separate PR after this is merged, I would totally be open to considering replacing the std::Result with anyhow::Result, i.e. that all the functions get changed from Result<T, Box<dyn Error>> to anyhow's Result<T> - should make it a lot easier!

Phew! I initially thought You wouldn't like the idea (I assumed that using anyhow could have some other backlash in the Wluma program), so I spent days trying to do it one by one, but, in the end, I gave up and used it anyhow, but I'm glad you liked my idea ☺️.

not that it is super important, but just reading the ash changelog, I think the devs intended you to replace ::builder() with ::default() and keep all the other code as is, i.e. in this case:

        let app_info = vk::ApplicationInfo::default()
            .application_name(&app_name)
            .application_version(WLUMA_VERSION)
            .engine_name(&app_name)
            .engine_version(WLUMA_VERSION)
            .api_version(VULKAN_VERSION);

I see, so it was intentionally done; I just found it somewhere as a reference to use default (read it in the changelogs as well), so I tried using it, and there were no errors being displayed, and it worked successfully. So I didn't question it. (I even asked ChatGPT for confirmation, but... well it failed.)

I think you might have lost one of the two extensions here, and from the patch you linked earlier today I think they found a more correct patch in this case:

image

Ah! My bad, its not that i forgot about this one but i couldnt find the updated version for KhrExternalMemoryCapabilitiesFn and i think i missinterpretted it and assumed that both are inside vkGetPhysicalDeviceProperties2 🫣. But still Thank you for finding that error will fix it soon.

From the quick glance, I think I prefer the changes made to this file in #86 (comment) , except I like your solution to the error types, not to introduce a ton of .expect() calls. Would you like to try to converge your work and the work in #86 (comment) ?

Yeah, Sure! I will do that in a couple of days.

Rishik-Y commented 2 weeks ago

Also Just wanted some tip for you @maximbaz , How do you usually find which one is which after the dependency is updated. For the past few days i have been struggling to figure out which one is which and what can be used for the replacement as such.

Since i was trying to update wlroots.rs, it became so messy to the point, i had to restart all over again today, just so i can make sense out of it πŸ˜΅β€πŸ’«.

maximbaz commented 2 weeks ago

Could you explain a bit what you mean by which is which after the dependency is updated? With an example, perhaps? :upside_down_face:

Rishik-Y commented 2 weeks ago

Could you explain a bit what you mean by which is which after the dependency is updated? With an example, perhaps? πŸ™ƒ

Like in wlroots.rs, EventQueue is still EventQueue (Small changes) However, they removed Main in v0.30 for Wayland Client, And I have to figure out ways to make it work with proxy (Totally changed) or Display, which was used in v0.29.5; instead of that, I am now having to use Connection for v0.31 (Mediocre change)

Stuff like that is where I have to read the entire documentation to see if there is any difference. Takes a lot of time, and even if I read, sometimes I still don't get what I need to do next.

maximbaz commented 2 weeks ago

Ahh I see what you mean now... There is no easy answer, I would start from the changelogs or release notes if they are there, and fall back to normal documentation otherwise. Many dependencies follow semantic versioning scheme, where just by looking at the version change from e.g. 2.5.3 to 2.6.0 you'd know that no changes to the code are necessary and that 2.6.x only brings new features and doesn't break any existing ones, buut as you have already figured out with wayland libs, sadly not everything follows it.

Rishik-Y commented 2 weeks ago

Ahh, I see; thanks for your explanation. I actually didn't know properly about semantic versioning till now. (Probably because most don't follow it)

Rishik-Y commented 2 weeks ago

Hello @maximbaz, I have re-updated Vulkan.rs code And I think everything properly has been re-written. I don't think there should be any error here (I have become thorough with the Vulkan.rs code) But could you still check once in case I missed something?

Rishik-Y commented 2 weeks ago

Let's revert to ok_or, potentially map_erroring it if needed.

Thank you for pointing out everything in detail. I will do just that now! πŸ”₯

That comment seems useful even if I don't remember the defaults πŸ˜… Lets keep it πŸ™

Oh sorry!πŸ˜… Will change it back!

It's okay to keep it for now, but I suppose you want to undo this before final merge πŸ˜‰

Yeah will do. πŸ˜€

Rishik-Y commented 2 weeks ago

Do you want to remove these commented parts, here and in a couple of other places, or you are experimenting with them?

Ah! Sorry, I forgot about that, I was commenting out some of my code in case if while changing my code it breaks I can come back and revert it.

Rishik-Y commented 2 weeks ago

Hey @maximbaz , I have successfully made all the changes you asked to revert back. Can you again reconfirm whether I missed something or made some mistake?

Rishik-Y commented 2 weeks ago

Okay I am back to Doubting myself, Do you want me to send you a screen recording later? (Or should I send something like verbose logs of wluma while running) To confirm it's working?

maximbaz commented 2 weeks ago
export RUST_LOG=trace
cargo run

Run it with wlroots.

Look for this log lines: https://github.com/maximbaz/wluma/blob/7184d0adb78775369163b1b222db15baa0748b7d/src/predictor/controller.rs#L198

luma is the value of the screen retrieved from wlroots and processed via vulkan. If you move back and forth between a black and white windows, you should see luma value change. If this happens, we are good :)

Rishik-Y commented 2 weeks ago

Unfortunately, My wlroots still doesn't work πŸ₯². Wluma only works when set to none in the compositor.

I ~assumed~ Hoped if i updated ash to v0.38 it may have an effect but it didn't, well i will continue to try later to update wayland-client and wayland-protocol.

Here is the screen recording i took to send.

https://github.com/user-attachments/assets/84d2bf20-ca77-4816-8f15-f726b4126839

https://github.com/user-attachments/assets/e615adff-0a77-4e2f-a328-b300fe4c094e

I used my study lamp to light up in front of my camera. In case you are wondering, the brightness is at the top beside the clock. And its changing based on light intensity so i think the code is working.

What do you think?

maximbaz commented 2 weeks ago

vulkan part is only involved when you use wlroots, so when you set capturer=none, you are also disabling the entire vulkan part, it doesn't get executed on your computer :wink:

At the same time I think the code change in this PR is very reasonable and I want to merge your very good progress, so that you can proceed with your awesome work from a fresh PR. Your contributions are very appreciated!

I'll ask a friend to test this in the coming days, plus I'm sure some people are running the latest code from main branch, so we will hear if this broke something.

Rishik-Y commented 2 weeks ago

omg, I totally went after a different thing till now; well, all in all, at least I learnt something. Thank you for accepting the PR. I will continue on this PR moving forward.

PS: Just realised I can at least moving forward eliminate vulkan.rs for not being the culprit on not working on my Hyprland

maximbaz commented 2 weeks ago

Ohh, if you asked this before and I mislead you into thinking that vulkan might be a culprit, I am sorry! (and still grateful for your PR :grin: )

maximbaz commented 2 weeks ago

PS: Just realised I can at least moving forward eliminate vulkan.rs for not being the culprit on not working on my Hyprland

I still believe this comment https://github.com/maximbaz/wluma/issues/111#issuecomment-2266043457 to be true, hyprland has the "stable" version of that protocol and we still use the "unstable" one because of the old wayland-client dependency. As soon as we update wayland-client, the current code like use wayland_protocols::wlr::unstable::export_dmabuf... will become use wayland_protocols::wlr::stable::export_dmabuf (unstable -> stable) or something like this, and then it will work again on hyprland.