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

Closed
wants to merge 10 commits into from
Closed

Added optional wait variance #967

wants to merge 10 commits into from

Conversation

shawarden
Copy link
Contributor

Added an optional argument to wait to the amount of time wait can vary between between the 2 specific values. 2nd 'vary' argument is ignored if value is less than 1st 'time' argument. This more closely resembles parse.py's description of wait as having a range value and it's handy to have for automation obfuscation.

Added an optional argument to wait to the amount of time wait can vary between between the 2 specific values. 2nd 'vary' argument is ignored if value is less than 1st 'time' argument. This more closely resembles the wiki's description of wait as having a range value.
Copy link
Owner

@sezanzeb sezanzeb left a comment

Choose a reason for hiding this comment

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

Thanks a bunch! A new unit-test would be very nice

Comment on lines 550 to 555
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

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

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

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

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.

inputremapper/injection/macros/macro.py Outdated Show resolved Hide resolved
Removed redundant _resolve calls in task and renamed `vary` variable to max_time so it makes more sense.
moved resolver into task so that changes in incoming values are respected
Shortcircuit optional argument comparison to ignore if not specified to be more consistent with other optional arguments.
Forgot to check is max_time is int or None
Added testing of `wait` function.
"""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

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.

shawarden and others added 4 commits October 21, 2024 20:05
Crude wait testing as we're rapidly reaching my limits in python vs the time I have for this.
@sezanzeb
Copy link
Owner

Merged in #986

@sezanzeb sezanzeb closed this Oct 25, 2024
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.

2 participants