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.

Sign up for GitHub

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 optional wait variance #967

Closed
wants to merge 10 commits into from
Closed

Added optional wait variance #967

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9b9a659
Added optional wait variance
shawarden Oct 6, 2024
59d91b6
Removed _resolves & renamed varible
shawarden Oct 7, 2024
7126912
moved resolver into task
shawarden Oct 7, 2024
cb38c90
Optional max_time defaults to None
shawarden Oct 8, 2024
74df211
Forgot to check is max_time is int or None
shawarden Oct 8, 2024
80f6c38
Added wait testing
shawarden Oct 10, 2024
5dd9295
crude test runtime
shawarden Oct 21, 2024
1bec762
unit testing wait macro
shawarden Oct 21, 2024
82361d8
Floating time ranges
shawarden Oct 21, 2024
d01fa92
float was being overridden by int
shawarden Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions inputremapper/injection/macros/macro.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import copy
import math
import re
import random
from typing import List, Callable, Awaitable, Tuple, Optional, Union, Any

from evdev.ecodes import (
Expand Down Expand Up @@ -540,12 +541,18 @@ async def task(handler: Callable):

self.tasks.append(task)

def add_wait(self, time: Union[int, float]):
def add_wait(self, time: Union[str, int], vary: int=0):
"""Wait time in milliseconds."""
time = _type_check(time, [int, float], "wait", 1)
time = _type_check(time, [int], "wait", 1)
Copy link
Owner

@sezanzeb sezanzeb Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What is the reason for removing float as supported datatype? If this feature is released and someone used wait(10.5), then I believe their macro will suddenly crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to make the wait macro accept a variable as input as it either didn't or I'd broken it somehow.

vary = _type_check(vary, [int], "wait", 2)

async def task(_):
await asyncio.sleep(_resolve(time, [int, float]) / 1000)
if _resolve(vary, [int]) > _resolve(time, [int]):
time_vary = random.randint(_resolve(time, [int]), _resolve(vary, [int]))
else:
time_vary = _resolve(time, [int])

await asyncio.sleep(_resolve(time_vary, [int, float]) / 1000)
Copy link
Owner

Choose a reason for hiding this comment

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

_resolve makes rpc calls. Though I doubt it would be noticeable by a user, on paper it would most likely be a lot better for performance if _resolve was called once for vary and time, and then the result stored in a local variable within task to reference it later on, instead of resolving time redundantly three times. It also reduces the risk of race conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair. When I made the initial changes the vary > time comparison didn't work and my knowledge of python isn't spectacular.

Copy link
Owner

@sezanzeb sezanzeb Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

you need to resolve them in task. _resolve looks up variables, and they can change during runtime. If you do it outside of _task they are resolved when the macro is parsed, and then never again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved the _resolve into _task. I'm not sure how to set the default value for max_time to be None as it throws an error comparing 'NoneType' and 'int' when the second argument isn't set. Again, my python is not amazing.

A slightly off-topic aside: In testing I found a way to test if macro variable is set or not by if_eq($var, None, then=unset_macro, else=set_macro). Would it be worth amending the examples documentation to illustrate it as a roundabout if_unset() function? Using wait with an unset variable can softlock keys if you have something like key_down(a).wait($unsetvariable).key_up(a) as the wait dies so key_up(a) never fires and having the macro always set unsetvariable first negates changing the value elsewhere.

Copy link
Owner

@sezanzeb sezanzeb Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

as it throws an error comparing 'NoneType' and 'int' when the second argument isn't set.

yeah, you would need to do something like if max_time is not None and max_time > ...

this way, you are not comparing 'NoneType' and 'int', because it never reaches the comparison if max_time is None.

Do you mean something like

if_eq(
  $unsetvariable,
  None,
  then=set(unsetvariable, 1000),
  else=?
).key_down(a).wait($unsetvariable).key_up(a)

?

Copy link
Owner

@sezanzeb sezanzeb Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

unittests are used in pipelines, like this one: https://github.com/sezanzeb/input-remapper/actions/runs/11427747182/job/31792217401 (Click on "Run tests" to see them)

They are used, so that we immediately see if something broke with our changes. In that case, github will display an error icon for us, like here:

image

Other than that, people rarely look at the test-output. And nobody is going to look at the output of a test each time they make a commit, to verify if things still work.

This is what I mean with "automated". Unittests should automatically check if the result is as expected (time within an acceptable margin of error), and if not, raise an exception (self.assertLess does that for us in the example below)

Here is how a test can work. It passes if 5 iterations of wait sleep between 45ms and 55ms on average.

    async def test_wait_1(self):
        mapping = DummyMapping()
        mapping.macro_key_sleep_ms = 0
        macro = parse("repeat(5, wait(50))", self.context, mapping, True)

        start = time.time()
        await macro.run(self.handler)
        time_per_iteration = (time.time() - start) / 5

        self.assertLess(abs(time_per_iteration - 0.05), 0.005)

Copy link
Owner

Choose a reason for hiding this comment

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

Automated tests are extremely important and every project should have them.

Copy link
Owner

Choose a reason for hiding this comment

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

I'll finish the pull request later if you don't have more time, that's fine. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've re-added the float inputs for the required time and as well as the optional max_time and carried that through the entire function while also making sure it didn't get flattened by the various ints. Also added debug output to the add_wait macro but it's not reflected in testing.

Using your unit testing it looks like they're all within desired +/-0.005s limits. All but a double variable retrieval are under 0.001 withwait($a,$b) breaking 0.002 even on my ancient work laptop.

Copy link
Owner

@sezanzeb sezanzeb Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If you remove the time randomization from add_wait, your tests will still pass, therefore they are not able to verify if the randomization works.

I have made some changes and a pull request to your branch: shawarden#1. In shawarden@dc98ddb you can see how I ended up designing the tests.

Once you merge that one into your main branch, I'll merge this pull request.


self.tasks.append(task)

Expand Down
Loading