nortikin / sverchok

Sverchok
http://nortikin.github.io/sverchok/
GNU General Public License v3.0
2.26k stars 233 forks source link

Proposed refactoring #225

Closed wclyffe closed 10 years ago

wclyffe commented 10 years ago

Hi guys,

First, great work on this addon as parametric modeling was sorely missing in Blender.

As I became intrigued and tried to wrap my head around what's possible, I decided to have a look at the code, which is growing at a frantic pace.

Anyway, in the process of trying to understand for myself how all that is working, I ended up forking and refactoring a bit: https://github.com/wclyffe/sverchok/tree/refactor The details of what have been done are in the commit log: https://github.com/wclyffe/sverchok/commit/a5d1b16f21c319cb555439534c383c6280f738ca

Essentially moving nodes in their own directories and changing how the nodes are imported in main init file. Apart from the imports the files contents should be untouched

On my end, all seem to be working as before, but I must say I haven't tried all the nodes...

Before I continue and begin to eventually refactor further, I would like to know if you are interrested, and if that is the case, some input would be greatly welcomed.

Again, you have done a great job and I certainlly hope that Sverchok will continue to extend what's possible with Blender

ly29 commented 10 years ago

The bpy.utils.register_class puts the class in bpy.types. But the bpy.types path is specified as an ugly way to work around the problems you have been solving with proper import structure instead of from module import *

Also get_socket_type should be updated to use bl_idname instead s m v to make more it future proof and to simplify that code, anyway that change is needed for more socket types, but since I didn't write that code I havn't spent time to change since it has been working for now.

It should be like this in the future.

def get_socket_type(node, inputsocketname):
    return node.inputs[inputsocketname].links[0].from_socket.bl_idname
ly29 commented 10 years ago

Other problems are with Formula2 node that also abused the import system by dumping the whole math module into local namespace to make sin etc available.

zeffii commented 10 years ago

node_script_templates folder definitely needs to move into the sverchok folder, it is relied on in the Text Editor menu plugin and the node_Script node.

https://github.com/nortikin/sverchok/blob/refactor/sverchok/utils/text_editor_submenu.py#L25-L28

https://github.com/nortikin/sverchok/blob/refactor/sverchok/nodes/generator/script.py#L87-L94 and in https://github.com/nortikin/sverchok/blob/refactor/sverchok/nodes/generator/script.py#L160-L167 too

zeffii commented 10 years ago

i propose to expose a path string through sv_tools.py or whatever , sv_path to avoid the hassle of path finding like this

def dirup(path, n):
    for i in range(n):
        path = os.path.dirname(path)
    return path

sv_path = dirup(os.path.realpath(__file__), 3)

sv_path would be constant for that session.

zeffii commented 10 years ago
Traceback (most recent call last):
  File "C:\blender_trunk\2.71\scripts\addons\sverchok\utils\sv_tools.py", line 52, in execute
    speedUpdate()
  File "C:\blender_trunk\2.71\scripts\addons\sverchok\..\sverchok\data_structure.py", line 1001, in sp
eedUpdate
    do_update(itertools.chain.from_iterable(update_list), ng.nodes)
  File "C:\blender_trunk\2.71\scripts\addons\sverchok\..\sverchok\data_structure.py", line 958, in do_
update
    nods[nod_name].update()
  File "C:\blender_trunk\2.71\scripts\addons\sverchok\..\sverchok\nodes\vector\interpolation.py", line
 147, in update
    out = eval_spline(spl, t, t_corr)
  File "C:\blender_trunk\2.71\scripts\addons\sverchok\..\sverchok\nodes\vector\interpolation.py", line
 85, in eval_spline
    n = bisect.bisect(tknots, t, lo=0, hi=len(tknots))-1
AttributeError: 'function' object has no attribute 'bisect'
ly29 commented 10 years ago

Check that it imports bisect from python and not the bisect node.

zeffii commented 10 years ago

yeah it's importing from modifiers

zeffii commented 10 years ago

https://github.com/nortikin/sverchok/commit/dfa371e276238dde0fab7b3abadf4db5e26e6843

wclyffe commented 10 years ago

@ly29

@ly29 @zeffii

ly29 commented 10 years ago

@wclyffe You are right, I looked at the wrong fork, my fault.

https://github.com/nortikin/sverchok/blob/refactor/sverchok/data_structure.py#L1028-L1085

wclyffe commented 10 years ago

@ly29 about bl_idname : if i'm not mistaken, that's what goes into bpy.types flat namespace. If so, bl_idname should follow some sort of name convention, or at least be prefixed to avoid some future potential headaches. By the same token, the nodes class names should follow suit. what about, with Foo being the category, Bar the node name :

ly29 commented 10 years ago

@wclyffe It most definitely should be like that. We started with Sv prefix once we learned about the problem but havn't been strict enough about it... Doing a big rename offering an inplace upgrade system is the right solution but before that is really workable we should split the actual processing from update function since that is a function meant for ui update events. That is the next big task I think, it is becoming urgent.

wclyffe commented 10 years ago

@ly29 And the code concerning ui udpate would be: https://github.com/nortikin/sverchok/blob/refactor/sverchok/data_structure.py#L682-L1010

zeffii commented 10 years ago

node_script_templates is used in 2 different places, I prefer a folder for ../node_scripts/templates which refers to what they are used for rather than where they are used, not a text_editor subfolder.

ly29 commented 10 years ago

That is code for ensuring that nodes update gets called in right order. The order those functions generate can be used in the future. Blender calls the nodes in order they where created by default so we have impose our own system to ensure valid execution.

zeffii commented 10 years ago

what about

sverchok
sverchok/node_scripts/
sverchok/node_scripts/templates/ <-- this is intended to get newcomers up to speed
sverchok/node_scripts/user_names/  <- we keep our own prototypes here

fits logically with

sverchok/..
sverchok/nodes/..
sverchok/node_scripts/..
sverchok/utils/..
..etc..

or sverchok/nodes/scripts/node_scripts/..

nortikin commented 10 years ago

PLEASE!!!!! make working init.py lay on root folder. i want user to download Sverchok from github and install as usual addon. PLEASE

zeffii commented 10 years ago

the way to go might be an Installer addon. to place the sverchok folder inside /addons one time, then the addon can be updated similar to how it worked before the refactor but with option to update to dev version or only to new stable versions.

wclyffe commented 10 years ago

@nortikin I can and will do that if you insist, you are the owner of the addon after all. But first, let me explain why I did package the addon like that:

I understand your concern, and will do as you wish.

But you should think about it. It might be marginaly more convenient for the user, but a deal more painfull to develop and maintain in the future. An alternative, might be to have a 'distribute' folder in the root, with pre-zipped files. Then those could be hyper-linked directly against.

zeffii commented 10 years ago

fetching a zip and only unpacking certain folders is straight forward iirc.

wclyffe commented 10 years ago

@zeffii I thought a bit about the updates : stable or dev Say the master branch is tagged for the stable releases, then dev is just the tip/head of the branch. I had a look at the github API and it allows to query for tags and return a direct link to a zip/tarball of the chosen tag

ly29 commented 10 years ago

@wclyffe A common workflow is to install the zip directly from github. Just download and install directly from blender add on settings, it isn't what the zip is meant to be but it is very convenient... As long as there is simple way to download Sverchok as a install ready zip file it is all good I think.

zeffii commented 10 years ago

An alternative, might be to have a 'distribute' folder in the root, with pre-zipped files. Then those could be hyper-linked directly against.

we'd need to automate the zip procedure when current branch has been tagged as stable, a cron job for a server bot maybe.

wclyffe commented 10 years ago

@ly29 The possibility to install directly from blender settings by pointing to the zip did escape me.

My point was more about what the repository is meant to be : it is primarly an environment in which to develop. Distributing the addon with all the added cruft developper might need, in my mind, is not optimal.

A solution to this dilemma, might be to have a distribute folder and a package script to produce the zipped, installation ready file and point to that as a save as link in the readme (which now appear on the front page without scrolling)

wclyffe commented 10 years ago

@zeffii Travis-ci.org offers that for open-source project I think

zeffii commented 10 years ago

@nortikin I think @wclyffe has very good points, good opportunity to do this the correct way now, more convenient for development. Our users will be bright people..

ly29 commented 10 years ago

@wclyffe I agree about what the repository should be, the way to produce the install ready zip sounds good to me. :+1:

zeffii commented 10 years ago

once we document the installation process changes, and implement the update mechanism again all people have to do is press update anyway.

I could make a simple Sverchok installer addon, as a bootstrap device to help simpliiy the first install

wclyffe commented 10 years ago

I didn't anticipated the changes to happen that quickly... This refactoring is still clearly unfinished. Some things are currently broken and that was to be expected. Changes are needed and we migth as well take our time to discuss and find a solution that works for everyone, users and developpers

wclyffe commented 10 years ago

I investigated a little bit on how to produce the addon.zip. Github has a release feature, including changelogs and binary assets (i.e. addon.zip), as well as an API to query for those assets. By hooking the repository with Travis-ci, a build service integrating with gihub, we could produce the assets in question automagically when taging a release. In the future we could even compile docs.md files to a nice pdf. Some info on all that:

zeffii commented 10 years ago

I've thought about this for a bit now, and have made small changes locally to fix nodes/generator/script.py i need node_script_templates (or some variation of this name) to be inside sverchok folder somewhere. My preference goes to sverchok/node_scripts/templates/, any objections?

wclyffe commented 10 years ago

@zeffii Not from me. Do you want me to move it there? Or will you move it?

zeffii commented 10 years ago

If you could please! node_script_templates to sverchok/node_scripts/ as subfolder templates so sverchok/node_scripts/templates/ preserving for now the subdir structure of username-folders and .py files.

Maybe it's time to setup my local Git correctly for this project :)

zeffii commented 10 years ago

btw, the addon.zip way is going to be awesome :)

wclyffe commented 10 years ago

@zeffii

nortikin commented 10 years ago

@wclyffe

nortikin commented 10 years ago

so, development brench ==> refactoring/sverchok/nodes master brench ==> master/nodes

this way i guess or not do it too complex and put init file to root, because if we change something, we eny time could put it back. I afraid, releases haedake will be long-lasting or take too much time. Now it works well for users and .gitignore file there

zeffii commented 10 years ago

Good documentation will explain this, and I will make sure of that.

nortikin commented 10 years ago

@zeffii how you will handle one name everytime? with overwriting of old zip file?

nortikin commented 10 years ago

@zeffii i tryed now, it warning me Duplicate tag name so, no way to make one-name release

nortikin commented 10 years ago

@zeffii or we can delete release every time... maybe and create new one manually... isn't ti headake? when it was zip archive on site, that i manually updated, i found that i am too lazy to create it often. with master git it more often. or let it be. ok. first time user will download, make folder copy-paste and so on. Ok, i dont mind. in same time i don't see real benefits of init file in subfolder.

Or, could we make init file as soft link to subfolder of real sverchok? that way we could make user install with zip and in the ither way install subfolder itself... am i right?

zeffii commented 10 years ago

@nortikin - it seems like a headache, but it won't last long. @wclyffe seems to know what he's doing.

As far as I understand, Travis cl.org "archiver bot " can name the archive anything we need. We force it to name the archive sverchok.zip. it overwrites the existing archive whenever we tag a snapshot as stable. This is an automated process, because yes we are lazy to update .zip ourselves, and there is a framework exactly for this situation.

@wclyffe can explain better -- sorry if i'm making things confusing. The end result is simple, but the process can be difficult to explain.

nortikin commented 10 years ago

@zeffii yes, maybe i didn't understand it right.

Sverchok not loading and not reporting anithing in terminal... how to find what is what there? manually searching?

zeffii commented 10 years ago

@nortikin look in the enabled list, it will say something like 'sverchok-master' not found. untick it. download sverchok-master, drag subfolder 'sverchok' into addons. then enable. works here. (this is what we do until we have an sverchok.zip archive in the sverchok root.

nortikin commented 10 years ago

sorry, i was changed stupidly sv_tools.py file that is why it didn't registered

zeffii commented 10 years ago

Okay! cool.

wclyffe commented 10 years ago

@nortikin I just pushed a packaging script (run from the command line) which produce a sverchok.zip (in the distribute folder) installable from within blender. The way it could work:

So all in all:

I know this is confusing. And it is a work in progress, that's why I made a refactor branch, to be merged when ready with the master.

nortikin commented 10 years ago

why it making screenshots? or is it only with me?

nortikin commented 10 years ago

ok, thanks

nortikin commented 10 years ago

where are new range int/float nodes? in what folder?