Uploading this script on behalf of Alban Denoyel & Bart Crouch for review.
Bart Crouch can commandeer the patch later if he likes.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Feedback inline, one question though...
This is packing all data into the blend, then saving as - wont this leave the file packed?, in that case the user might not notice... and save their work back to the original blend without meaning to. (Maybe I misunderstand here), but overall this is my main concern.
| io_online_sketchfab/__init__.py | ||
|---|---|---|
| 1 | should have typical shortened GPL header we have at the top of all bundled addons. | |
| 18 | This is a really large python library (which I didn't include in the patch), we need to consider how to bundle large 3rd party python modules. I can see why its done for the purpose of getting the addon working & reviewed but having each addon include its own large dependancies isn't so good. At the very least this could be moved to: Then at least this is a common location any addon can import from. | |
| 37 | This generic function could be added to: ./release/scripts/modules/bpy/utils.py | |
| 56 | better have the name global uppercase, along with SKETCHFAB_API_URL since its referenced more then once. | |
| 58 | use with open(...) as file: format here, same goes for all other uses of open. this is best practice for py nowadays | |
| 60 | Suggest to handle this a bit differently.... something like... if not file exists:
return
try:
load the file
except:
import traceback
traceback.print_exc()This way if the file exists but can't be read, it wont fail silently - making it hard to figure out whats going on. | |
| 74 | This should probably be a set(), to de-duplicate on the fly. | |
| 100 | Its possible pack fails, for example if the file is missing, currently this will give a rather ugly error popup and the script fails halfway through. See the C code for this - newPackedFile if (file < 0) BKE_reportf(reports, RPT_ERROR, "Unable to pack file, source path '%s' not found", name); So, best pass operator reports to prepare_assets and report a warning if pack fails for any files. We should probably have a general utility function for converting a python exception into a warning. nevertheless this should be handled somehow. | |
| 116 | strange API use, better do os.path.dirname | |
| 120 | better use os.path.join() | |
| 141 |
| |
| 161 | better use os.path.splitext | |
| 264 | I don't think this menu is needed, better use a dynamic popup? | |
| 316 | typically we import all props used in the script... rather then bpy.props.StringProperty each time. | |
| 324 | our convention us to use all-caps for enum values. (this goes for all enums here). | |
Comment Actions
Something I didn't mention was threading with Python is known not to work 100% with Blender,
However I've observed it can work reasonably well when 2 threads are at least not attempting to change blender data at once - this is still something we'll need to double check is working well enough to include in a release.
Comment Actions
Hi,
About your question:
"This is packing all data into the blend, then saving as - wont this leave the file packed?, in that case the user might not notice... and save their work back to the original blend without meaning to. (Maybe I misunderstand here), but overall this is my main concern."
The script "saves as" with a different filename so there should not be an issue of saving back work to the original blend is there? (sorry I'm not very familiar with Blender scripting).
Let me know if something needs to be updated.
Otherwise all your remarks (excluding questions I left as comments to your inline notes) are taken into account in the attached revision.
Best,
PA{F77994}
| io_online_sketchfab/__init__.py | ||
|---|---|---|
| 1 | Sure. Can you send what you want to be included and I'll add it. | |
| 18 | This library is a slight fork from this: I think having it in a module every add-on can import is a great idea, as it is very generic and many other people could benefit from using it. One thing I would like to point out: I had to fork the main library trunk, because the library has one "import uuid" which cause a c++ runtime warning on c++. Do you have plans for fixing this in 2.7? Would be great/easier to maintain if we don't deviate from the library's master. Let me know how you want to proceed with this. | |
| 37 | Sure let me know if you do and I'll update the script. | |
Comment Actions
Regarding packing,
This is a problem still...
- User has file which contains non-packed images
- Run this addon, modifies files and save-as is executed...
- their original blend file is untouched
- now they continue working and save their file.
- now their original file contains packed images.
We could add some option to pack-on-save, so it keeps the open file unaffected, since saving with packed data is generally a useful thing to be able to do - but this isnt some simple task so think its worth to consider alternatives.
- Dont pack - save the blend and zip images with it, When loading run bpy.ops.file.find_missing_files() on to re-link any absolute paths. (I think this is the best option).
- Save out to an interchange format, OBJ/FBX/Collada etc... - zip images with the file.
- Run a new instance of blender in an external process (windowless), pack and save the data - then exit the process, no changes to the users data is made.
| io_online_sketchfab/__init__.py | ||
|---|---|---|
| 1 | first 17 lines from io_export_after_effects.py is fine | |
| 18 | From what I understand the issue with importing uuid - with windows configuration, See: So looks like theres a workaround in Python we could apply, but Im not really sure how good/bad this is, or why Py devs are not considering this higher priority to resolve. | |
Comment Actions
Since there are some issues with requests, how much work is it to drop it as a dependency?
See replies: http://lists.blender.org/pipermail/bf-committers/2014-February/042870.html
Comment Actions
Hey,
Not including requests would be a pain, as it would require recoding a url lib from scratch, which is a hard task. Totally worth the extra 300kb in my opinion.
I went for call an external blender process as it will be more flexible down the line than just zipping textures.
The updated sources are attached.
Let me know what you think.
Comment Actions
urllib is a part of python,
See:
http://download.blender.org/release/Blender2.70
In the latest testbuild:
blender-2.70-testbuild1-linux-glibc211-x86_64/2.69/python/lib/python3.3/urllib
Comment Actions
Yes but unfortunately it does not support well file attachement, which is a major pitfall and the reason for the existence of 'requests' and other external http libs.
Comment Actions
@Pierre-Antoine Passet (pierreant-p) - there were 2 issues regarding bundling requests which aren't yet addressed.
- Including CACERTS (It was stated that these are out dates though I can't confirm this).
- No fixed version (they don't tag their releases in github).
Could you reply to these comments? (reply here can work too, I posted on the mailing list since this is an issue for platform maintainers).
Comment Actions
Hi @Campbell Barton (campbellbarton),
Regarding the questions about 'requests', here are my thoughts:
- Including CACERTS >> Honestly, giving the nature of the data blenders users are going to send from and to blender, I wouldn't worry so much about having a slightly outdated certificates (assuming this is the case, I didn't check it out) >> It's that's still any issue, I'm glad to update the library with newer ones if someone provides them. Since we're slightly forking for the main trunk anyway, I don't think this is an issue. We can also create a pull request and ask for the library maintainer to update de CACERTS >> If that's still an issue, we can still consider to include this library only for the sketchfab plugin?
- Releases/tags: >> It seems they do tag their stable releases: https://github.com/kennethreitz/requests/releases, the latest stable version being held in the master trunk. >> If people are unconfortable with this, we could fallback to the latest tagged version "2.2.1" instead of the newer "2.3.0" but I'm not sure if it's worth it.
Let me know what your think,
Best,
PA
Comment Actions
Hi @Pierre-Antoine Passet (pierreant-p)
- re: CACERTS, did you check if they are even required? I rather not disregard security topics as not important but to stay completely clear of them and not even get involved with distributing certificates with blender.
- Good to know they do tags.
Think its really too late to include this addon with the release, but we should make it a priority to resolve topics for next release.
so...
- Resolve UUID issue with windows, we're dropping XP support, so perhaps this means we can resolve the issue with windows runtime.
- Remove any need for using a forked requests
- Include Python binary with Blender so we can do multi-processing OR, make blender threadsafe for Python.
This is something for blender to support (not suggesting you guys should have to do all this), but think we needs to do this before the addon can really integrate well.
Comment Actions
Hey,
Update:
- removal of uuid dependency on Windows (causing runtime error)
- removal of outdated cacerpts.pem
PA{F79144}
Comment Actions
Update on review:
The solution for replacing UUID, but a less involved fix can work (noted inline).
Most other issues I found were while testing.
Unsaved files
If you attempt to upload from a file which isn't saved, it gives a strange error
Error occured while preparing your file: 'size'
... after that try save and press upload again, it says to wait for the current upload, you have to restart blender for it to work again.
If unsaved files are unsupported then best report that, however if an upload does fail for some reason, it should probably be handled better, at least disable the upload state.
Ignoring Unsaved Changes
Uploaded blend's are from the last saved version of the file,
Currently if you make a quick change and press upload, the change will be ignored unless you press save first.
How to handle this will is tricky, but think a simple solution could be to do a save-copy, write a temp blend, upload it and delete.
Upload Popup
While popup's are used in blender sometimes, they typically are used for confirmation, the way the popup works in this case can happen while you're in the middle of using Blender and easy to accidentally quit.
Really dont think this is good, suggest:
- Report the upload is complete in the info header (not a popup but text that displays for a time), show the upload is complete in the panel and have a button to view it.
- Make the operator blocking, don't attempt interactivity while it runs, then show a popup when its finished (can be a dialog rather then a menu so you dont quit accidentally).
Panel Placement
The new panel should probably go in the toolbar (not view properties) and have its own tab.
| io_online_sketchfab/__init__.py | ||
|---|---|---|
| 43 | This is adding a custom importer which executes on every import for every script that blender does once the addon loads, I wouldn't accept this in an official blender addon. All you need to do is load filepost and overwrite its choose_boundary function. eg: def choose_boundary():
from uuid import uuid4
return uuid4().hex
requests.packages.urllib3.filepost.choose_boundary = choose_boundary | |
| 250 | -b and --background are the same thing. | |
| io_online_sketchfab/filepost.py | ||
| 1 ↗ | (On Diff #1152) | There should be no need for this file, see comments above. |
Comment Actions
I have taken into account the user interface and usability recommendations.
filepost.py is removed and we use a less hacky hack to bypass the uuid bug on windows.
Comment Actions
Update. requests is now bundled with blender and have prepared the script to commit.
I've made some edits to this script, which Ill commit shortly.
Summary of changes.
- don't store internal variables as RNA (use sf_state global).
- errors in packing were failing silently, now they show in the terminal and popup (though the error text could be improved).
- make panel only show in object mode.
- ran over pep8 style checker.
Some fixes.
- textures with None images failed.
- add exception handling around image.pack since it can fail.
- token_reload was defined twice.
- SKETCHFAB_EXPORT_DATA_FILE, was written twice for no reason.
Comment Actions
If blender is installed into a system path, the sketchfab addon would fail to upload a model that has never been saved.
The following version fixes that :
Comment Actions
@Clément (Wiz), Committed rBA8b0864a5e3f7b7d1849898de2623dbb79fc4179e
replaced sys.argv[7] with sys.argv[-1] otherwise no changes,
if more args are passed python has an argparse module to better handle this stuff.