Added Turntable animation exporter by iamfunsho · Pull Request #210 · simlu/voxelshop

iamfunsho · 2016-11-23T08:31:58Z

Please review and test and let me know what you think, so after that i can post a tutorial on Twitter and also improve the standard image render export to also include support for background color and watermark.

Minor Hotfix

iamfunsho · 2016-11-23T08:33:21Z

Arrrr merge conflict, i knew this will happen, will fix

simlu

I really like what you did so far!

(1) Export seems to be slow in general. I don't think it would be hard to parallelize. Thoughts?

(2) We should be able to set the image resolution and ratio and the camera angle in the exporter and position the camera so that the whole model is visible. You can use HackedFrameBuffer to set the size. Just need to figure out how to position the camera automatically.

(3) The bounding box should be disabled for the rendering.

That's it so far! Good work, but lots more to do :)

simlu · 2016-11-24T02:49:29Z

PS4k/src/com/vitco/core/EngineViewPrototype.java

    }
    // get the image rendered in container in high quality
-    public BufferedImage getImage() {
+    public BufferedImage getImage() throws Exception {


What exception does this throw?

IOException while loading the font for the watermark

Can you make the Exception that is thrown explicit then? I.e. IOException instead of Exception?

simlu · 2016-11-24T02:51:12Z

PS4k/src/com/vitco/core/EngineViewPrototype.java

    }
+
+    // get camera position
+    public final float[] getCamPosition() { return this.camera.getCamPosition(); }


Don't write these as one line, write as

public final float[] getCamPosition() { return this.camera.getCamPosition(); }

Since it's a single line of code, i tends to write such methods like that (Another bad Habit) will change.

Yeah, I don't like long lines very much. There is a reason swift > objective-c :)

simlu · 2016-11-24T02:52:31Z

PS4k/src/com/vitco/core/EngineViewPrototype.java

+    // get camera position
+    public final float[] getCamPosition() { return this.camera.getCamPosition(); }
+
+    public BufferedImage getImage(final Color bgColor, final boolean addWatermark) throws Exception { return container.getImage(bgColor, addWatermark); }


Why do the variables need to be final? I think you can remove the final keyword.

just an agreement that i will never modify it, will change it if you don't like it

Yeah I think final should only be used when there is a reason for it. That's how I'm usually handling it.

Will change, but i will also love to know what your reasons are

Mostly "Shorter is better and faster to read".

The answer here is pretty good: http://stackoverflow.com/questions/154314/when-should-one-use-final-for-method-parameters-and-local-variables

simlu · 2016-11-24T03:11:22Z

PS4k/src/com/vitco/layout/content/menu/MainMenuLogic.java

+        turnTableImageSeq.addComponent(new SeparatorModule(""));
+        turnTableImageSeq.addComponent(new CheckBoxModule("add_background_color", "Add Image background color", false));
+        TextInputModule TISBackgroundColor = new TextInputModule("background_color", "Image Background Color: e.g #ffffff", "#000000", true);
+        TISBackgroundColor.setVisibleLookup("export_type=turntable_image_sequence&turntable_image_sequence.add_background_color=true");


I don't think this should be hidden... (bad user experience)

I didn't hide it at first but later i thought it will be easier to understand cause if there is no background color the image background will be transparent. And also other applications does that,but if you don't like it, i can change it.

I had hiding components a lot initially and it was confusing as hell :)

simlu · 2016-11-24T03:13:25Z

PS4k/src/com/vitco/layout/content/menu/MainMenuLogic.java

+                            protected Object doInBackground() throws Exception {
+
+                                // export color render (image)
+                                progressDialog.setActivity("Processing turntable image sequence", true);


The progress bar should display the actual progress. Shouldn't be hard to do.

Actual progress of entire export instead of each images?

Yes. There are other places in the code where I do that. You need to have a counter and also provide the max value.

simlu · 2016-11-24T03:14:30Z

PS4k/src/com/vitco/layout/content/menu/MainMenuLogic.java

+        // ---------------
+
+        // add "turntable animation" export
+        FieldSet turntableAnim = new FieldSet("turntable_animation", "Turntable Render (*.gif)");


There should only be one "turnable" exporter with the option gif and separate images. The exporter options is already crowded enough!

The two does the same thing in different ways. The animation (.gif) is mostly useful if you want to quickly showcase your model on social media but the image sequence will be helpful to those who want to use the render in third party application e.g video/image editor. Merging the two together will either create a mess, give user less options or just make the list longer. BTW here on linux i experience issues of not being able to scroll in the export dialog (colladae). Another option is to remove it from export dialog and include it in the toolbar/header bar like how QBCL does it.

Mhmmm, I guess we have to consider what the use cases are. I think we should keep it in the export dialog but also have quick access buttons.

I definitely like the idea of merging it. And I disagree: I think we can achieve this without creating a mess giving users less options.

Can you create a bug report for that issue with screenshot please? Thanks!

simlu · 2016-11-24T03:17:05Z

PS4k/src/com/vitco/layout/content/menu/MainMenuLogic.java

+
+                        // ===========
+
+                        // -- export turntable animation


The export logic should be separated out of this file.

Should we make an export class(abstract)/interface.

Yes. I'm thinking all the exporters will be separated out eventually. But for now just doing it with this one is fine. I really need to rework the MainMenuLogic class. It's a giant mess :)

simlu · 2016-11-24T03:29:42Z

Also I get interesting transparency issues. Somehow black is made transparent for some reason in the animated gif?

iamfunsho · 2016-11-24T04:22:27Z

Well it depends on how many files are being exported.
I like the idea of image size / resolution but i dont like the idea of setting the camera's view position before the shot, i wanted to implement it but i realize that it's going to give users less freedom and they won't be able to render from whatever angle they like, so i dropped it.
I kinda like the render with the bounding box, users should disabled the bounding box before render if they don't like it. But if you insist on disabling it before render or adding an option to disable it in the exporter, it ok.

iamfunsho · 2016-11-24T04:24:14Z

About the transpareny issue, can you please explain more and tell me how to replicate it.

iamfunsho · 2016-11-24T04:29:33Z

Most issues can only be looked into when you are done with travis

simlu · 2016-11-24T06:35:37Z

Regarding transparency issue: Here is a sample export http://i.imgur.com/locIHtR.gif

simlu · 2016-11-24T06:38:05Z

(1) Animation even with the slowest setting it takes 30 seconds and this computer is FAST. If we allow to define the view port size that can be improved by setting it smaller default. It's pretty large currently.

(2) I'm not sure I understand? They get the same freedom? With the improvement that they don't accidentally cut of the head. It's gonna be especially nice when we show the camera angles in the main view port.

(3) I think it should be an option in the exporter then.

simlu · 2016-11-24T06:39:01Z

I'm pretty much done with the heavy file changes for travis. You can branch off of the ci-prep branch and continue working!

simlu · 2016-11-24T06:40:08Z

Another issue I just noticed: The file doesn't get closed after the export completes. This needs to be fixed!

iamfunsho · 2016-11-24T07:13:20Z

Unfortunately i can't view the video but i think i also experience a similar issue just now (all black are transparent) and i think the problem is coming from the fact that if no background color is set then black will be used as the transpareny color therefore all black pixels are rendered transparent by the gif encoder. There can be a workaround but am assuming it to be "dirty" or we just go with a semi transparent background (which won't be nice).

simlu · 2016-11-24T07:19:24Z

Well Gif Doesnt Support Alpha Transparency. So The Workaround should Be Easy. Find Color Not In Image, set transparent Pixels To That and Use That for Transparency. Can You Set What The Gif Encoder uses?

simlu · 2016-11-24T07:20:29Z

Ideally you should be able to tell it to use transparency...

iamfunsho · 2016-11-24T07:28:29Z

Gif supports transparency, that why i said it dirty, it will require me to go pixel by pixel to check which color is in the image

iamfunsho · 2016-11-24T07:29:50Z

Yeah i can set what color the gif encoder uses.

iamfunsho · 2016-11-24T07:31:28Z

How were you able to know the file is not getting close?

simlu · 2016-11-24T07:31:45Z

Alpha transparency != transparency

simlu · 2016-11-24T07:32:17Z

I couldn't delete it :)

iamfunsho · 2016-11-24T07:34:07Z

About the animation speed, it can be improved but i dont currently have any ideas, gifs take some time even with ImageMagick.
We seriously need to fixed the issue of head getting cut off (not showing full image), will look into that.

iamfunsho · 2016-11-24T07:35:39Z

You mean Alpha transparency like png? Unfortunately gif doesn't support that, My mistake.

simlu · 2016-11-24T07:36:41Z

No I don't think it does? Reference?

iamfunsho · 2016-11-24T07:38:00Z

Typo, i meant to type "doesn't" :D

iamfunsho · 2016-11-24T08:00:18Z

The Animation exporter needs improvement, the best things to avoid are are loops that will worsen performance. So i think we should remove gif transparency background support since it's not really alpha transparency and set black as the default background color instead. What did you think.

simlu · 2016-11-30T04:06:09Z

How are things going here? It looks really promising, but needs more work. Need help?

iamfunsho · 2016-11-30T06:43:42Z

No, thanks. I will improve it.

simlu and others added 2 commits November 21, 2016 19:55

Merge pull request simlu#207 from simlu/develop

4c15be3

Minor Hotfix

Added Turntable animation exporter

87cd3ed

funabab added 2 commits November 23, 2016 09:42

--allow-empty-message

3b95900

Merge branch 'develop' into export-turntable

8471db9

simlu requested changes Nov 24, 2016

View reviewed changes

iamfunsho mentioned this pull request Nov 25, 2016

Proper use of Bounding Box #211

Closed

Conversation

iamfunsho commented Nov 23, 2016

Uh oh!

iamfunsho commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simlu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamfunsho Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamfunsho Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simlu commented Nov 24, 2016

Uh oh!

iamfunsho commented Nov 24, 2016

Uh oh!

iamfunsho commented Nov 24, 2016

Uh oh!

iamfunsho commented Nov 24, 2016

Uh oh!

simlu commented Nov 24, 2016

Uh oh!

simlu commented Nov 24, 2016

Uh oh!

simlu commented Nov 24, 2016

Uh oh!

simlu commented Nov 24, 2016

Uh oh!

iamfunsho commented Nov 24, 2016

Uh oh!

simlu commented Nov 24, 2016

iamfunsho commented Nov 23, 2016 •

edited

Loading

iamfunsho Nov 24, 2016 •

edited

Loading

iamfunsho Nov 24, 2016 •

edited

Loading

iamfunsho commented Nov 24, 2016 •

edited

Loading

iamfunsho commented Nov 24, 2016 •

edited

Loading