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

Added Async Version #31

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Added Async Version #31

wants to merge 5 commits into from

Conversation

cbkcbk
Copy link

@cbkcbk cbkcbk commented Jun 26, 2024

Added a async version of the project, this uses dotnets async/await features and not threading. This should make it play better with single threaded applications and example web projects that use async/await heavily. i have also updated the workflows to hopefully add the async version to nuget and zip file

@caesuric
Copy link
Owner

Hey, thanks for contributing, and for using the library!

I have some questions and thoughts on this one.

  1. Can you explain to me the benefits of this for single-threaded applications? There is already a way to call Plan() and Step() synchronously so I am a little confused.
  2. If the changes do make sense, we should integrate them into the main project rather than creating a "fork" within the repo, mostly because I don't want to maintain two copies of the project going forward. Maybe this can be a feature of the Step() function. Can you help me understand what this PR helps with?

Thank you!

@caesuric caesuric self-requested a review June 26, 2024 13:26
@cbkcbk
Copy link
Author

cbkcbk commented Jun 26, 2024

Hi, The idea is that it should be possible to await functions from Actions, ensuring compatibility with single-threaded and multithreaded applications (such as Blazor WASM witch is only single threaded at the moment, enabling calls to a web API as an action without blocking the UI thread )

I made it so this will compile as its own nuget package and dll, so others can chose between the normal package and this async version.

@ray2k
Copy link

ray2k commented Jul 5, 2024

This would be pretty nice- working with web apis I need to do async/await in sensors and actions

@caesuric
Copy link
Owner

caesuric commented Jul 5, 2024

I'll take a deeper look at this and figure out how to integrate it into the existing version. I don't want to maintain two copies in the codebase with unnecessary code duplication, but it sounds like this is a feature that's wanted.

@cbkcbk
Copy link
Author

cbkcbk commented Jul 5, 2024

After using this for a few days and conducting some tests, I believe I have found a better way to integrate this into a single project. I'm working on it right now and should be able to submit it for review in a few hours. Additionally, I uncovered what seemed to be a bug in the A* calculation that led to infinite loops while attempting to find a viable plan.

@cbkcbk
Copy link
Author

cbkcbk commented Jul 5, 2024

i have commited my working version now.

@caesuric
Copy link
Owner

caesuric commented Jul 6, 2024

Thank you so much! I'll take a look again as soon as I'm able. :)

@caesuric
Copy link
Owner

Just wanted to follow up on this -- I have a solution that I think is a little more in line with how I want the library to be structured, and will be implementing it soon. Thank you for bringing this issue to my attention, and my apologies that this has taken a while.

@blooblahguy
Copy link

I think this would be such a great core function of the project, is there any update on this? It's been a couple months so if it's too much time implement, no problem, but I just want to make sure I choose the right library for my needs.

@caesuric
Copy link
Owner

caesuric commented Oct 8, 2024

I think this would be such a great core function of the project, is there any update on this? It's been a couple months so if it's too much time implement, no problem, but I just want to make sure I choose the right library for my needs.

Apologies, I have been distracted quite a bit by work and other projects. I'll try to incorporate this soon. There are some issues with incorporating as is so it needs some tweaks, but I will make it more of a priority.

@caesuric
Copy link
Owner

caesuric commented Dec 1, 2024

I (finally) added a PR version here that I am hoping will suffice and which will not break existing code: #35

If anyone gets a chance, please let me know if this version is okay, and I can adjust if needed. Thank you! @cbkcbk @ray2k @blooblahguy

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