Update ash & some other deps #118

Rishik-Y · 2024-09-15T13:01:05Z

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 · 2024-09-15T13:03:37Z

Wait really sorry
Forgot about cargo.lock

Rishik-Y · 2024-09-15T13:14:54Z

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 · 2024-09-16T16:02:04Z

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 · 2024-09-16T16:28:01Z

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 · 2024-09-16T16:43:56Z

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 · 2024-09-16T16:54:04Z

Sorry to ask but
I just saw #86 (comment)
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 · 2024-09-16T18:29:17Z

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.

maximbaz · 2024-09-16T18:42:42Z

Cargo.toml

 xdg = "~2.5"
 dbus = "~0.9"
+anyhow = "1.0"


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!

Still relevant 🙏

src/frame/vulkan.rs

maximbaz · 2024-09-16T18:55:33Z

src/frame/vulkan.rs

+            enabled_extension_count: instance_extensions.len() as u32,
+            pp_enabled_extension_names: instance_extensions.as_ptr(),
+            ..Default::default()
+        };


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) ?

Rishik-Y · 2024-09-16T19:44:50Z

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:

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 · 2024-09-16T19:46:35Z

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 · 2024-09-16T19:54:54Z

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

Rishik-Y · 2024-09-16T20:41:50Z

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 · 2024-09-16T20:47:51Z

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 · 2024-09-17T12:45:33Z

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)

Updated to ash v0.38

Rishik-Y · 2024-09-17T16:43:35Z

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?

maximbaz

Looking good overall, some minor findings!

maximbaz · 2024-09-17T16:54:54Z

src/frame/vulkan.rs

@@ -129,7 +136,7 @@ impl Vulkan {
            &device_memory_properties,
            vk::MemoryPropertyFlags::HOST_VISIBLE | vk::MemoryPropertyFlags::HOST_COHERENT,
        )
-        .ok_or("Unable to find suitable memory type for the buffer")?;
+        .expect("Unable to find suitable memory type for the buffer");


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

maximbaz · 2024-09-17T16:55:34Z

src/frame/vulkan.rs

@@ -220,7 +230,7 @@ impl Vulkan {
            self.device.unmap_memory(self.buffer_memory);
            self.device
                .reset_fences(&[self.fence])
-                .map_err(|e| Box::new(e) as Box<dyn Error>)?;
+                .expect("Failed to reset fences");


Same here to revert to map_err

maximbaz · 2024-09-17T16:55:57Z

src/frame/vulkan.rs

+        let image_memory = unsafe {
+            self.device
+                .allocate_memory(&image_allocate_info, None)
+                .expect("Failed to allocate memory")


map_err 🙏

maximbaz · 2024-09-17T16:56:07Z

src/frame/vulkan.rs

-        }
+            self.device
+                .bind_image_memory(image, image_memory, 0)
+                .expect("Failed to bind image memory");


map_err 🙏

maximbaz · 2024-09-17T16:56:19Z

src/frame/vulkan.rs

+        let frame_image = unsafe {
+            self.device
+                .create_image(&frame_image_create_info, None)
+                .expect("Failed to create image")


map_err 🙏

maximbaz · 2024-09-17T16:57:59Z

src/frame/vulkan.rs

-                .queue_submit(self.queue, &[submit_info.build()], self.fence)
-                .map_err(|e| Box::new(e) as Box<dyn Error>)?;
+                .queue_submit(self.queue, &[submit_info], self.fence)
+                .expect("Failed to submit queue");


map_err 🙏

maximbaz · 2024-09-17T16:58:03Z

src/frame/vulkan.rs

            self.device
                .wait_for_fences(&[self.fence], true, FENCES_TIMEOUT_NS)
-                .map_err(|e| Box::new(e) as Box<dyn Error>)?;
+                .expect("Failed to wait for fences");


map_err 🙏

maximbaz · 2024-09-17T16:58:14Z

Cargo.toml

 xdg = "~2.5"
 dbus = "~0.9"
+anyhow = "1.0"


Still relevant 🙏

maximbaz · 2024-09-17T16:58:34Z

config.toml

-name = "keyboard-dell"
-path = "/sys/bus/platform/devices/dell-laptop/leds/dell::kbd_backlight"
+name = "keyboard-MSI"
+path = "/sys/class/net/enp4s0/enp4s0-2::lan"


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

maximbaz · 2024-09-17T16:59:01Z

src/frame/vulkan.rs

            .queue_family_index(queue_family_index)
-            .queue_priorities(&[1.0])
-            .build()];
+            //p_queue_priorities: [1.0f32].as_ptr(),


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

Rishik-Y · 2024-09-17T17:35:48Z

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 · 2024-09-17T17:38:55Z

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 · 2024-09-17T18:09:52Z

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?

maximbaz

From my perspective this looks like a very clean PR now, I have just a couple minor suggestions, other than that I'm good to merge. Thanks! 🙏

I don't see why it wouldn't work, but just to be super explicit I will repeat that I personally don't have a hardware to validate this, so I trust your test results, and then the users who would run the code from main branch.

Cargo.toml

src/frame/vulkan.rs

Co-authored-by: Maxim Baz <[email protected]>

Rishik-Y · 2024-09-17T19:27:49Z

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 · 2024-09-17T19:36:31Z

export RUST_LOG=trace
cargo run

Run it with wlroots.

Look for this log lines:

wluma/src/predictor/controller.rs

Line 198 in 7184d0a

log::trace!("Prediction: {} (lux: {}, luma: {})", prediction, lux, luma);

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 · 2024-09-17T20:00:21Z

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.

2024-09-18.01-07-26.mp4

2024-09-18.01-19-50.mp4

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 · 2024-09-17T20:09:19Z

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 😉

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 · 2024-09-17T20:12:14Z

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 · 2024-09-17T20:17:04Z

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 😁 )

maximbaz · 2024-09-17T20:20:05Z

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 #111 (comment) 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.

Rishik-Y added 3 commits September 15, 2024 18:09

Added anyhow to manage errors

1728991

Add files via upload

4ed07fb

Update vulkan.rs

86b10e2

Rishik-Y added 2 commits September 15, 2024 18:41

Add files via upload

fef25f0

Add files via upload

63d077c

maximbaz reviewed Sep 16, 2024

View reviewed changes

Rishik-Y added 5 commits September 17, 2024 01:49

Add files via upload

1468533

Add files via upload

103c079

Add files via upload

72ecfd7

Add files via upload

dc7a87e

Add files via upload

6223a3a

Add files via upload

085f587

Add files via upload

ccbd81b

Rishik-Y added 4 commits September 17, 2024 18:56

Add files via upload

939a754

Made Changes to vulkan.rs

d0dce89

Add files via upload

f46dd10

Updated Vulkan.rs with even more modification

35f327c

Rishik-Y added 2 commits September 17, 2024 22:02

Add files via upload

961607f

Vulkan.rs updated successfully

60c22fe

Updated to ash v0.38

maximbaz requested changes Sep 17, 2024

View reviewed changes

Rishik-Y added 2 commits September 17, 2024 23:36

Add files via upload

b8c3185

Add files via upload

abe1014

maximbaz reviewed Sep 17, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/frame/vulkan.rs Outdated Show resolved Hide resolved

Rishik-Y and others added 2 commits September 18, 2024 00:39

Update src/frame/vulkan.rs

4191b2d

Co-authored-by: Maxim Baz <[email protected]>

Changed anyhow to "~1.0"

dc71267

Co-authored-by: Maxim Baz <[email protected]>

maximbaz changed the title ~~Updated ash from v0.37 to v0.38~~ Update ash & some other deps Sep 17, 2024

maximbaz merged commit 08b4896 into maximbaz:main Sep 17, 2024
2 checks passed

Update ash & some other deps #118

Update ash & some other deps #118

Conversation

Rishik-Y commented Sep 15, 2024

Rishik-Y commented Sep 15, 2024

Rishik-Y commented Sep 15, 2024

Rishik-Y commented Sep 16, 2024

maximbaz commented Sep 16, 2024

Rishik-Y commented Sep 16, 2024 • edited Loading

Rishik-Y commented Sep 16, 2024

maximbaz commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rishik-Y commented Sep 16, 2024

Rishik-Y commented Sep 16, 2024

maximbaz commented Sep 16, 2024

Rishik-Y commented Sep 16, 2024

maximbaz commented Sep 16, 2024

Rishik-Y commented Sep 17, 2024

Rishik-Y commented Sep 17, 2024

maximbaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rishik-Y commented Sep 17, 2024

Rishik-Y commented Sep 17, 2024

Rishik-Y commented Sep 17, 2024

maximbaz left a comment

Choose a reason for hiding this comment

Rishik-Y commented Sep 17, 2024

maximbaz commented Sep 17, 2024

Rishik-Y commented Sep 17, 2024

maximbaz commented Sep 17, 2024

Rishik-Y commented Sep 17, 2024 • edited Loading

maximbaz commented Sep 17, 2024

maximbaz commented Sep 17, 2024

Rishik-Y commented Sep 16, 2024 •

edited

Loading

Rishik-Y commented Sep 17, 2024 •

edited

Loading