Auto-gen continuation - Githubissues

vidartf commented 7 years ago

Further work on the autogen branch, continuing from #94. This PR includes many changes, including:

Update to three.js r85.
Allow enums to be autogenerated.
Add configurable assignment functions (needed for properties without a setter, like vectors and matrices).
Refactor base classes such that most objects are Widgets instead of DOMWidgets. As all DOMWidgets have a Layout model, this cuts the total number of models in half. The preview ability is retained by adding a new DOMWidget class that is created via _ipython_display_ method on the three widgets.
Add controller classes / renderer class similar to that in master. This helps setup interactive scenes with full configuration of camera/controller + live updates to changes.
Reinstate PlainGeometry. Syncing all vertices/face/uvs/normals on all geometries is prohibitively expensive. By readding PlainGeometry as a Geometry class that syncs all these properties, the user can better control when it wants to access the underlying attributes.
Many other smaller changes.

honorabel commented 7 years ago

looking like awesome work @vidartf! will review over the next few days.

in the future, if you're able to break up your pr's into smaller chunks, it will make it much easier to review them faster and get them merged in quick. if a certain change set does not depend on future work, go ahead and file a PR to get those changed reviewed sooner :)

vidartf commented 7 years ago

@honorabel Yes I know (you have my apologies and my sympathy). I was working on this towards a real use case to ensure I got something in the end that did what I wanted, and as such all the different changes became intertwined (r85 upgrade fixes sometimes occurred after other changes had been done). This made it hard to split the different changes into logically separate parts.

vidartf commented 7 years ago

Looking over the diff, I'll see if I can separate out some of the major independent components (e.g. like enum autogen).

abelnation commented 7 years ago

nah, don't worry about it for this one. i went ahead and added comments

On Tue, Jun 20, 2017 at 8:41 AM, Vidar Tonaas Fauske < notifications@github.com> wrote:

Looking over the diff, I'll see if I can separate out some of the major independent components (e.g. like enum autogen).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jovyan/pythreejs/pull/105#issuecomment-309799853, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMNdvdaqFnnwIretbhoBcVKIPJhgqy0ks5sF-gigaJpZM4N_fgC .

-- Abel Allison abel.allison@gmail.com

vidartf commented 7 years ago

Alright, I've added some commits addressing the comments (unless otherwise commented). Thanks for the quick review :)

vidartf commented 7 years ago

Merge and iterate?

jasongrout commented 7 years ago

Merge and iterate?

I think it's fine to merge into a branch and iterate.

jasongrout commented 7 years ago

(you have merge permissions - feel free to merge and iterate, especially since it's going into a branch and not master)

SylvainCorlay commented 7 years ago

Why not master? We can always branch out when we need extra release of the previous version?

jasongrout commented 7 years ago

Why not master? We can always branch out when we need extra release of the previous version?

We could do that too. I saw this PR was already going into a branch, though.

vidartf commented 7 years ago

I thought the plan was to iterate on the autogen branch until it was on par with the master branch, and then update and finally merge #88.

jupyter-widgets / pythreejs

Auto-gen continuation #105