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

Fix default camera in instance mode #119

Merged
merged 4 commits into from May 4, 2017

Conversation

Projects
None yet
3 participants
@islemaster
Collaborator

islemaster commented Oct 20, 2016

Attempts to access the lazy-created default camera property in instance mode were leading to errors, and it was also interfering with p5's createGraphics() as reported in #104. This PR registers an initialization method with p5 as proposed in #46 and creates the default camera there instead. A number of changes were necessary to make this viable, but I think it's worth it, as we can gradually migrate away from lazy-created properties to using the init method for everything.

p5.js upgrade

I had to upgrade the p5.js version used by the examples; the ability to register an 'init' method was released in 0.4.24 and we were on 0.4.21. I went ahead and upgraded all the way to 0.5.4. This means we now depend on p5.js >= 0.4.24, but I'm not sure we've previously recorded that dependency anywhere. Opinions on where to put it?

Examples page changes

It seems registering an init method was not working on the examples page because it would load p5.js once on the page, but would eval p5.play with each example - and also because, for some reason, it was clearing all registered methods prior to eval'ing the examples. I verified that removing this does not cause us to register init too many times.

One nice side effect of this is that you can debug p5.play on the examples page now, which was previously difficult since it was being eval'd.

I think the biggest risk is possible state bleed between examples, but if that's possible I haven't seen it happen yet. We seem to create a new p5 instance for every example anyway.

Fix up sprites_with_sheet example

I'm not sure how this was working before - we were using the result of a loadJSON call on another line inside preload(), which is not allowed. To fix, I'm using its result inside a callback - I know it's an advanced technique, but it's probably necessary here if you want to generate a spritesheet in preload() using asynchronously-fetched JSON.

islemaster added some commits Oct 20, 2016

Make examples page load p5.play directly
It seems registering an init method was not working on the examples page because it would load p5.js once on the page, but would eval p5.play with each example - and also because, for some reason, it was clearing all registered methods prior to eval'ing the examples.  I verified that removing this does not cause us to register init too many times.

One nice side effect of this is that you can debug p5.play on the examples page now, which was previously difficult since it was being eval'd.

I think the biggest risk is possible state bleed between examples, but if that's possible I haven't seen it happen yet.  We seem to create a new p5 instance for every example anyway.
Fix sprites_with_sheet example
Not sure how this was working before - we were using the result of a loadJSON call on another line inside `preload()`, which is not allowed.  To fix, I'm using its result inside a callback - I know it's an advanced technique, but it's probably necessary here if you want to generate a spritesheet in preload() using asynchronously-fetched JSON.
Create camera in registered init method
This converts the default `camera` to be created in an initialization method we register with p5 instead of using the elaborate lazy-p5-property workaround we had before.

This will be our first progress toward fixing #46 and should entirely fix #104.
@molleindustria

This comment has been minimized.

Show comment
Hide comment
@molleindustria

molleindustria Oct 20, 2016

Owner

I can update the examples on the "official" page if things changed.
To this date I still have no idea of how instance mode works.

Also, should I set up a github web page that can be updated collectively (right now it's just static stuff on my server)?

Owner

molleindustria commented Oct 20, 2016

I can update the examples on the "official" page if things changed.
To this date I still have no idea of how instance mode works.

Also, should I set up a github web page that can be updated collectively (right now it's just static stuff on my server)?

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Oct 20, 2016

Collaborator

Note to self: Although tests are passing, I'd like to clean up the new warning You just changed the value of "camera", which was a p5 function. This could cause problems later if you're not careful. before merging.

Collaborator

islemaster commented Oct 20, 2016

Note to self: Although tests are passing, I'd like to clean up the new warning You just changed the value of "camera", which was a p5 function. This could cause problems later if you're not careful. before merging.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Oct 20, 2016

Collaborator

@molleindustria we are already serving these at molleindustria.github.io/p5.play, such that anyone with write access to the repository should be able to update it by running update-gh-pages.sh. Back in March @toolness suggested a way to have p5play.molleindustria.org point to the version at github.io, using a CNAME DNS record.

It would be a small step now to get that environment to auto-deploy any time master is updated - I do exactly that in this other repo to keep the "play in browser" feature current with the master branch.

Collaborator

islemaster commented Oct 20, 2016

@molleindustria we are already serving these at molleindustria.github.io/p5.play, such that anyone with write access to the repository should be able to update it by running update-gh-pages.sh. Back in March @toolness suggested a way to have p5play.molleindustria.org point to the version at github.io, using a CNAME DNS record.

It would be a small step now to get that environment to auto-deploy any time master is updated - I do exactly that in this other repo to keep the "play in browser" feature current with the master branch.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Oct 20, 2016

Collaborator

Regarding the warning, I'm not sure I should fix in this PR; p5.play is legitimately replacing the p5.js method camera, present since 0.4.10 (September 2015) with its own default camera (not in the docs but used in the camera.js example), present at least since the initial commit in June 2015 ☹️. There just hasn't been a warning until now.

Collaborator

islemaster commented Oct 20, 2016

Regarding the warning, I'm not sure I should fix in this PR; p5.play is legitimately replacing the p5.js method camera, present since 0.4.10 (September 2015) with its own default camera (not in the docs but used in the camera.js example), present at least since the initial commit in June 2015 ☹️. There just hasn't been a warning until now.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 4, 2017

Collaborator

Haven't had a lot of feedback on this change, but it definitely fixes issue so I'm merging.

Collaborator

islemaster commented May 4, 2017

Haven't had a lot of feedback on this change, but it definitely fixes issue so I'm merging.

@islemaster islemaster merged commit 5c29bd1 into molleindustria:master May 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@islemaster islemaster deleted the islemaster:createGraphics-instance-mode branch May 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment