Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ofxAssimpModelLoader aiNode bugfix #1720

Merged
merged 3 commits into from
Dec 3, 2012

Conversation

julapy
Copy link
Member

@julapy julapy commented Dec 3, 2012

found a pretty big bug in ofxAssimpModelLoader where the aiNodes were being completely ignored.
this was breaking basic node transformations and animations.

this article shows how to render assimp data correctly,
http://www.lighthouse3d.com/cg-topics/code-samples/importing-3d-models-with-assimp/
and ive implemented a fix based on that.

also had to make a couple tweaks to the example models which were set on the wrong axis... im assuming to compensate for the previous incorrect behaviour.

ofxAssimpModelLoader still needs some more loving... it seems its only be setup to work for bone animations and a load of other functionality has been ignored.

@ofTheo
Copy link
Member

ofTheo commented Dec 3, 2012

awesome - thanks @julapy

+1 for assimp fixes.
every time I use it I seem to stumble on a bug or two.

would love better support for animation. I've never been able to get a animated model to load in OF ( minus the samples ones we ship with ).

okay to merge!

kylemcdonald added a commit that referenced this pull request Dec 3, 2012
ofxAssimpModelLoader aiNode bugfix
@kylemcdonald kylemcdonald merged commit ddc12f3 into openframeworks:develop Dec 3, 2012
@kylemcdonald
Copy link
Contributor

i needed these changes too. just tested this and it looks good!

@julapy
Copy link
Member Author

julapy commented Dec 3, 2012

@ofTheo, give animations a go now with the new fix, its working well for me.

i also find the naming of the classes confusing...
ofxAssimpModelLoader suggests its just a loader class but its also a renderer.
i think ofxAssimpModelLoader should be ofxAssimpModel
and ofxAssimpMeshHelper should be ofxAssimpMesh
and ofx3DBaseLoader is not even used...

im also looking at creating a ofxAssimpAnimation class that can deal with playing animations,
atm im re-writting the same code on different projects just to play the models and recon its something that should be included in the addon.

@ofTheo
Copy link
Member

ofTheo commented Dec 3, 2012

great - that all sounds good to me!
I'm going to be working heavily on iOS and 3D stuff the next few months, so will be able to help on it as well.

happy to get a branch going which we can both hack away on.

@julapy
Copy link
Member Author

julapy commented Dec 3, 2012

@ofTheo sounds good!

@kylemcdonald
Copy link
Contributor

awesome, these changes are super welcome @julapy. another feature that would be really nice is dynamic bone manipulation, which i've done a little work with and posted on the forum about last year.

@ofTheo
Copy link
Member

ofTheo commented Dec 4, 2012

also - to further complicate things.
there is assimp 3.0 http://assimp.sourceforge.net/lib_html/index.html

which is not backwards compatible.

@arturoc
Copy link
Member

arturoc commented Dec 4, 2012

i was going to suggest that since you are going to do a major rework perhaps it's worth to move to 3.0?

@julapy
Copy link
Member Author

julapy commented Dec 4, 2012

what if we moved away from ofxAssimpModelLoader addon and left it as is,
and then created ofxAssimp addon (correctly named this time) which would work with 3.0?
that way there would be two versions, ofxAssimpModelLoader addon for legacy projects,
and the new ofxAssimp addon which we could release pretty quickly as it doesn't depend on the older code,
that way the community can benefit from using it sooner.

@julapy
Copy link
Member Author

julapy commented Dec 4, 2012

@kylemcdonald do you have a link for the bone manipulation code?

@arturoc
Copy link
Member

arturoc commented Dec 4, 2012

we could also base ofxAssimpModelLoader on the new ofxAssimp to not have 2 different versions of the library

@bilderbuchi
Copy link
Member

I think it would be too confusing for users... you have an ofxAssimpModelLoader, and an ofxAssimp, which one to choose? then someone starts a fork without actually forking, one more version floating around (it happened with other ofx addons already).... (I think I'm being influenced by Python :P)

btw about compatibility: what does that entail, precisely? as far as I can tell, only the Assimp API changed, not some project file format or so. and since we are wrapping that into our OF-specific ofxAssimp API, this shouldn't matter as long as we change our (public-facing) API as little as possible, right?

I found

API COMPATIBILITY: - renamed headers, export interface, C API properties and meta data prevent compatibility with code written for 2.0, but in most cases these can be easily resolved

at http://sourceforge.net/projects/assimp/files/assimp-3.0/

@kylemcdonald
Copy link
Contributor

@julapy
Copy link
Member Author

julapy commented Dec 4, 2012

i think one big problem is that the naming of the addon and its classes is not consistent with what it does.
as agreed on above, ofxAssimpModelLoader should be renamed to ofxAssimpModel as its not just a loader but it also does rendering of the model. and if this is the case, we should probably also look at renaming the addon from
ofxAssimpModelLoader to ofxAssimp.

@bilderbuchi, i see your point about it getting confusion having the two version...

ofxAssimp should support the same API as ofxAssimpModelLoader, and anything redundant we can wrap up in the OF deprecation function and remove later on. so it should be straight forward to port over any ofxAssimpModelLoader dependent code to ofxAssimp. we should also just have the one version, so once ofxAssimp is ready, it will replace ofxAssimpModelLoader.

thoughts?

@bilderbuchi
Copy link
Member

I like ofxAssimp as a name. simple and to the point.
yeah I also thought of the deprecation functions for an easier transition.
Do you expect any difficulties with the assimp 2.x ->3.0 transition?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants