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

Jump to bottom

New timer functions with lambdas #2843

Closed
wants to merge 14 commits into from
Closed

New timer functions with lambdas #2843

wants to merge 14 commits into from

Conversation

Vurv78
Copy link
Member

@Vurv78 Vurv78 commented Nov 13, 2023 •
edited
Loading 

// simple timer
timer(1, function() {})

// timer with name
timer("name", 1, function() {})

// timer with name and reps
timer("name", 1, 2, function() {})

stoptimer("name")

This also changes the convention of lambdas being called from lua, now you need access to the E2 entity to call them. This could change since the context does store the actual chip entity on itself (context.entity)

@Vurv78
Initial lambda implementation
01103d8
@Vurv78
Add ops on creation
12cfb26
@Vurv78
New implementation
7e9913d 
* Functions are now tables, containing parameter signature, return type and inner function
* function:getParameterTypes()
* function:getReturnType()
* Don't reset global variables on `@strict`, fixing issue with top level locals not working as upvalues since they'd be reset by the runtime. I don't think this would actually affect anyone since you shouldn't be able to use variables before they're assigned, but it's `@strict` only behavior anyway.
@Vurv78
Fix missing parity change
641a8f2 
I'd already fixed this bug with functions that have return values but this was not fixed for functions without return values. Also added a test case for this.
@Vurv78
More tests, enforce returns at compile time
6aebc04 
* Add tests to ensure variadic parameters, void parameters and implicit parameters aren't allowed

* Fix lambdas potentially not returning at all codepaths like functions now are expected to do. Added a test for that.
@Vurv78
Merge branch 'master' into functions
631b801
@Vurv78
Merge branch 'master' into timer2
168a2a4
@Vurv78
Add timers via callbacks
db32af2
@Vurv78
Execute timer with chip entity, move ops
342f156 
* Ops will increase inside of the actual lambda's body instead of in ExprDynCall, which means it won't cost 0 ops to call them outside of an E2 chip.

* Lambdas outside of E2 will now need to be called with ENT:Execute, which now optionally takes a "script" and "args" corresponding to what you'd pass to the lambda. It will handle perf and everything for you.

* Remove Lambda:Call, UnsafeCall still exists in case someone wants to handle perf on their own in some extension.
@Vurv78
Add timer(snf)
1a78d0d
thegrb93
thegrb93 previously requested changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@thegrb93 thegrb93 left a comment

Choose a reason for hiding this comment

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

There needs to be a limit to the number of timers created since they are implemented very inefficiently in the engine

@Vurv78
Copy link
Member Author

Vurv78 commented Nov 14, 2023

E2 never had a timer limit though, I guess I could make it something large like 1k to avoid breaking things.

@Vurv78 Vurv78 marked this pull request as draft November 18, 2023 06:07
@Denneisk
Copy link
Member

I don't think getTimers should be deprecated, but a timerExists would be useful.

@github-actions GitHub Actions
Copy link

This pull request has been marked as stale as there haven't been any changes in the past month. It will be closed in 15 days.

@github-actions github-actions bot added the Stale label Dec 24, 2023
@Denneisk
Copy link
Member

it's over

@github-actions github-actions bot removed the Stale label Dec 25, 2023
@Vurv78
Merge branch 'master' into timer2
064fbaf
@Vurv78
Refactor, add per-chip limit
274325d 
* Fix error when chip quotas with timers active
* Make getTimers nodiscard, change deprecation message
* Optimize code to not create / remove timers needlessly
* Auto-remove timer with reps properly
@Vurv78
Decrease prf cost, clarify change
8d31764 
15 ops to call lambda -> 10 ops
@Denneisk
Copy link
Member

Denneisk commented Jan 2, 2024

we're back

@Vurv78 Vurv78 marked this pull request as ready for review January 2, 2024 05:13
@Vurv78
Copy link
Member Author

Vurv78 commented Jan 4, 2024

There needs to be a limit to the number of timers created since they are implemented very inefficiently in the engine

Forgot to say, implemented as 100 per chip

@Vurv78 Vurv78 requested a review from thegrb93 January 4, 2024 21:05
@Vurv78
Copy link
Member Author

Vurv78 commented Jan 4, 2024

I don't think getTimers should be deprecated, but a timerExists would be useful.

This is the only thing to be discussed, I was going to undeprecate it but now it's kind of odd since simple timers are leaking into the output since they use the same system... not sure if I like that so I kept it deprecated.

Also the name is not very namespaced, so maybe could change this behavior and have a timerList function?

Need to implement timerExists too

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 6, 2024

This pull request has been marked as stale as there haven't been any changes in the past month. It will be closed in 15 days.

@github-actions github-actions bot added the Stale label Mar 6, 2024
@Denneisk
Copy link
Member

Please don't let this die.

@github-actions github-actions bot removed the Stale label Mar 24, 2024
@Denneisk
Copy link
Member

Denneisk commented Apr 1, 2024

Is this ready to be merged or should I fork it and continue on it myself? Just want timerExists and...? Everything else seems to be solved.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 5, 2024

This pull request has been marked as stale as there haven't been any changes in the past month. It will be closed in 15 days.

@github-actions github-actions bot added the Stale label May 5, 2024
@github-actions github-actions bot closed this Jun 20, 2024
@Denneisk Denneisk mentioned this pull request Oct 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Denneisk Denneisk Denneisk approved these changes

@thegrb93 thegrb93 Awaiting requested review from thegrb93

Assignees
No one assigned
Labels
Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Vurv78 @Denneisk @thegrb93