Page MenuHome

Empty Image: add movie and image sequence
ClosedPublic

Authored by Brecht Van Lommel (brecht) on Dec 16 2013, 8:36 AM.

Details

Summary

So this patch allow the possibility to display movie or image sequences with and empty image.
I'ts a WIP. If the reviewers can help me it's welcome.
TODO:

  • max limit to the number of frame available.
  • UI maybe.

Thanks

Diff Detail

Event Timeline

Brecht Van Lommel (brecht) requested changes to this revision.Dec 16 2013, 8:43 AM

I don't think this is the right approach, the Empty should have an ImageUser so you can specify all the settings that come with that, not just the frame.

Sorry, not seen that you've write before I post the video.
What can do the image user more?
For auto refresh? Do we want that?
I've asked me before, I'm not sure that the usability will be improve if we add the image user.
It's not easy to use and understand, offset, start, frame, I think it's confusing for the artist who just want to display a specific frame from movie.
Maybe the artist don't want to synchronize the movie frame rate with the blender frame rate. then you only key-frame the frame field how you want, like I show at the end of the video.
Thanks for the extreme quick review by the way

The ImageUser UI is indeed confusing, but that is something we should fix, the same argument could be made for other places where it is used. Consistency is important here I think, the same way to specify image sequence parameters and multilayer images should be used everywhere.

Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Dec 17 2013, 10:57 PM

I post this unfinished patch to have help.
It's with ImageUser as Brecht ask.
What working:

  • I can see movie or image sequence
  • Go to a frame with the offset

What not working:

  • playback (ALT+A)(autorefresh check). need to "walk" in every object to chack for the ImageUser??
  • I've always this error "IMB_loadifffile: couldn't get mapping /MY_PATH".
  • Key frame the offset "Could not insert keyframe, as RNA path is invalid for the given ID (ID = OBEmpty.002, path = .frame_offset)"

For sure I make something wrong but I can't figure out.
I don't know if it is the right place to ask for help.
Thanks

Object is getting fairly bloated and I'm not so happy see an ImageUser added to every object (when this is only used for empties).

On the other hand I can't think of a better way to do this either.

source/blender/makesdna/DNA_object_types.h
281

Why was an image pointer added?

Campbell Barton (campbellbarton) requested changes to this revision.Dec 18 2013, 11:19 AM

We can at least make ImageUser a pointer? The ImageUser struct can be allocated in rna_Object_data_set in rna_object.c, when the type assigned is an image.

At some point we should figure out a way to add such data to objects without increasing the struct size, bug I guess another pointer isn't too bad.

release/scripts/startup/bl_ui/properties_data_empty.py
42

This line was removed, but it shouldn't be.

release/scripts/startup/bl_ui/properties_data_empty.py
42

Well it seems to be moved, but it should still be there even when ob.data is None, and at the top of the buttons.

Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Dec 18 2013, 1:21 PM

Image in struct Object was just a test to see if that solve my problem but doesn't.
Can you also orient me to solve the problem above.
Thanks

I don't have time at the moment to help fixing the issues in this patch, it will take me as much time as writing this from scratch, and that's not really an efficient use of time for me.

You should look at other auto refresh code to see how it works, and rna_ImageUser_path for the keyframing thing.

Brecht Van Lommel (brecht) requested changes to this revision.Dec 18 2013, 4:37 PM
Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Dec 19 2013, 12:17 AM

So now it work perfectly with auto refresh and cyclic.
Thanks Brecht to show me where I can found the solution.

Only one error isn't solve.
"IMB_loadifffile: couldn't get mapping /blend path".
I'll try to find out. A idea maybe.
BIG THANKS again for your help and sorry to use your time for small task.

I've try with a new blend file and don't have the problem, it seems that come from my test blend maybe corrupt.
So it work perfectly now.

Brecht Van Lommel (brecht) requested changes to this revision.Dec 19 2013, 5:12 PM
Brecht Van Lommel (brecht) added inline comments.
release/scripts/startup/bl_ui/properties_data_empty.py
42

This line of code is still missing, you can't change the empty draw type without this.

44

This change does not look correct, now you can't change to an image type and pick the image datablock?

54

Not sure why this line was moved under the if, it's also relevant when you don't use an image empty.

source/blender/makesdna/DNA_object_types.h
281

This should be "ImageUser *iuser;" and be dynamically allocated, as mentioned above.

Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Dec 19 2013, 9:46 PM

change ImageUser iuser to *iuser
UI fix

Currently the image user point is in a bit of an odd state.

  • Always added to any empty.
  • Not ensured to be valid when loading old blends.

This is confusing because its not clear if this is expected to be valid or not.

I'd suggest _not_ to add this for new empty objects, and only allocate it when setting the empty type to an image. Freeing it when changing the image type not to be an empty.

source/blender/blenloader/intern/readfile.c
5020

no need for a NULL check here.

source/blender/blenloader/intern/writefile.c
1538

*picky* space between ) { - all other cases too.

source/blender/makesdna/DNA_object_types.h
4

This is never freed and will cause a memory leak (try run blender with the -d argument).

Brecht Van Lommel (brecht) requested changes to this revision.Dec 20 2013, 3:17 PM
Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Dec 22 2013, 1:06 AM

Now image user is create only when it's an image empty and feed else (I hope/think "ob->iuser = NULL;")
I'll test if there is problem to open old blend file

Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Dec 22 2013, 2:48 AM

Now it can open old blend file

Would prefer just have BKE_object_empty_draw_type_set(), so as to avoid using RNA in readfile, rna_Object_empty_draw_type_set can simply wrap BKE_object_empty_draw_type_set

release/scripts/startup/bl_ui/properties_data_empty.py
44

If done right, there should be no need to check ob.image_user - blender can ensure if its an IMAGE type, the image_user is always set.

source/blender/blenloader/intern/readfile.c
5027

Theres no need to use RNA here, just set the value.

source/blender/editors/object/object_add.c
738

again, no need to use rna.

source/blender/makesrna/intern/rna_object.c
503

This will leak memory.

Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Dec 22 2013, 9:23 PM

Close to being finished.

release/scripts/startup/bl_ui/properties_data_empty.py
52

*picky* try avoid trailing whitespace.

source/blender/blenkernel/intern/image.c
2218 ↗(On Diff #400)

should be no need to check ob->iuser here since its now assured for any image-empty.

source/blender/blenkernel/intern/object.c
2577

its possible ob->iuser is already set, this will overwrite and leak memory.

source/blender/makesdna/DNA_object_types.h
288

best add comment here that this is assured to be non-null when set to image type.

Brecht Van Lommel (brecht) requested changes to this revision.Dec 23 2013, 6:25 PM
Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Dec 23 2013, 10:55 PM

Made the change request and done some clean up about tab and space.

Heres an updated patch with minor edits, Still not so happy to commit this into master, since now on animation playback its looping over all object in the blend just to check for empty-image-objects.

http://www.graphicall.org/ftp/ideasman42/image_empty.diff

Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Dec 24 2013, 8:53 AM

ah ok, empty_drawtype can be OB_EMPTY_IMAGE even if then object is not an empty.No need to cast ob->data? I don't quite anderstand what is ob->data is there documentation about it?
thanks

@Krantz Geoffroy (kgeogeo)

  • Checking ob->type == OB_EMPTY, is needed since you can set empty_draw_type on non empty objects (from python, try it!).
  • And theres no need to cast to a pointer to check ob->data is NULL.

Looks like other uses of ob->empty_drawtype == OB_EMPTY_IMAGE should check for ob->type.

Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Dec 25 2013, 1:46 AM

I hope it's the last one. I've learn a lot with all your advice, but I'm really afraid to disturb
because I make only small step, not checking every case. Sorry for that.
Thanks a lot for your patience.

Krantz Geoffroy (kgeogeo) updated this revision to Unknown Object (????).Jan 12 2014, 9:43 PM

rebase to the current git revision.

We need to come to some conclusion here on the patches behavior...

Personally I think its not acceptable that fairly obscure features like this slow down animation playback for all files by checking every object for every frame update.

Ideally Blender would keep a separate list of empty-objects that store movie images, so the overhead would be negligible.

This is a shame since @Krantz Geoffroy (kgeogeo) already spend quite some effort on this patch, but fast animation playback on large scenes is important too and not something to compromise lightly.

Brecht Van Lommel (brecht) updated this revision to Unknown Object (????).

Ok, what do you think about this? Using the depsgraph rather than looping over all objects an extra time.

LGTM, how about checking BKE_image_is_animated to avoid updating depsgraph state for every empty-image. (added function just now)

source/blender/blenkernel/intern/depsgraph.c
1959

could add BKE_image_is_animated check before running

source/blender/blenkernel/intern/object.c
2963

could add BKE_image_is_animated check before running

Committed now, thanks for your patience.