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

Add experimental support for instance mode #5

Merged
merged 11 commits into from
Nov 28, 2022

Conversation

Vindaar
Copy link
Contributor

@Vindaar Vindaar commented Nov 27, 2022

After my brain was refreshed from exercising I decided to take a stab at implementing this.

I'm pretty happy with the results! We add a type for a P5Instance which contains all symbols that are accessible from the instance macro. For each procedure that is to be callable, we generate them from the existing wrapper by using a new globalAndLocal macro that takes definitions and outputs two versions of them:

  • regular
  • one taking a P5Instance as first argument, that is to be emitted to JS as p5Inst.<name>(<args>) (this is important, as the procedures are methods of the instance, as far as I understand so far)

The new instance template shadows the existing sugar templates with versions that generate a new closure and add it to the corresponding field of the P5Instance object (remaining templates need to be added as wrapper templates as well as as fields to the P5Instance type definition).
Within these templates the body is checked for possible replacements from two sources:

  • all procedures that were wrapped in globalAndLocal (for nnkCall nodes { nnkDotExpr will be added later } )
  • all fields of the P5Instance type (for nnkSym / nnkIdent nodes)

This approach (that internally uses CacheSeqs to store the symbols) has the advantage that we don't convert too little / too many things.

See the example for what the resulting code looks like.

@Vindaar
Copy link
Contributor Author

Vindaar commented Nov 28, 2022

@pietroppeter I noticed you're not watching your own repository, so I guess it's possible you'll overlook this.

@pietroppeter
Copy link
Owner

pietroppeter commented Nov 28, 2022

@pietroppeter I noticed you're not watching your own repository, so I guess it's possible you'll overlook this.

Ah wow, indeed I got the notification only when you mentioned me. Now I should be watching it. Thanks so much for this! Need to look at it and give you feedback (first feedback is probably I will keep instance related code in a separate module instead of the module with the original bindings; edit: looked quickly at the implementation and it is evident why it needs to be in the bindings: we need to generate the two versions for global and instance mode).

@Vindaar
Copy link
Contributor Author

Vindaar commented Nov 28, 2022

The majority of the logic can be moved to a submodule though! I just put it there for convenience at the moment. And given that we use a CacheSeq for storing the CT information, it would be safe even to have the define the globalAndLocal macro in a submodule (technically using a regular Table at CT also works, but that apparently will break incremental compilation once that is a thing).

…tures

The added doc comments should explain well enough how it works:
- `globalAndLocal` wraps all procedures to also provide method taking
`P5Instance`. While doing so, all names are added to a CacheSeq
- an `instance` template that shadows existing helper templates such
that closures are generated and added to the field
- in each template call, the bodies are checked for possible
replacements of calls / symbols that need a `p5Inst` first argument
The types must be in a separate files now so that we can import them
in the instance logic file.
Also updates the instances example accordingly.
@Vindaar
Copy link
Contributor Author

Vindaar commented Nov 28, 2022

Just rebased on current master, moved the instance macro logic to a submodule and extended support for nnkDotExpr appearing in the instance template bodies.

In principle I consider this ready for merge, with the remaining optional TODOs left:

  • extend the P5Instance object to really contain all fields the real instance allows for (I just copied some of the globally defined wrapped variables)
  • make sure to put all procedures that require it under globalAndLocal
  • extend the instance template to not only include setup and draw. This also requires an addition to the P5Instance class for each addition, as each is stored as a closure in the object. If one decides to provide everything that is p5sugar.nim it could also be done with a macro in principle, or just via copy & paste. The code is pretty trivial after all (but I'm not even sure if all of that is valid for P5Instance).

@pietroppeter
Copy link
Owner

I will check this out and likely merge it. I want to test it out to have two instances in the same document and possibly create a nimib sugar for those (which includes automatically generated ids for divs). It seems reasonable to have only a partial subset of the functionalities now, we will add them as we go (most of the functionalities in the bindings are still untested for me, since they have no example where they are used).

@pietroppeter
Copy link
Owner

ok, I checked this and tested the two instances. In order for that to work the divName must be a cstring and not a string (there are multiple occurrences of string in the existing bindings, they will likely need to be all changed to cstring). I made the changes and update the instances example to have two copies of the get started example. works fine!

as another minor changes: used importjs instead of importcpp.

@Vindaar, hope you do not mind I committed directly here, seemed easier. If ok for you I would merge this, close the instance issue and open a new one with the improvements you listed above.

@pietroppeter
Copy link
Owner

ah, another thing I noticed is there was a import jsffi left in the examples. I removed it and examples seem to work fine. Is it something that we think might be needed in some cases for instance mode? if so maybe the better way would be to just export it from p5.

@Vindaar
Copy link
Contributor Author

Vindaar commented Nov 28, 2022

Ah, no. that import is just an accidental left over, because I started writing the prototype code in the example. Thanks for noticing. I'll take it out later.

edit: you can of course also remove it! I don't mind you committing here at all, no worries!

@pietroppeter
Copy link
Owner

Already removed. Ready to merge on my side.

@pietroppeter pietroppeter merged commit a081e14 into pietroppeter:master Nov 28, 2022
@pietroppeter
Copy link
Owner

merging, thanks again a lot for this great work @Vindaar !

@pietroppeter pietroppeter mentioned this pull request Nov 28, 2022
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