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

First part of removing states from models #125

Merged
merged 26 commits into from Aug 18, 2016

Conversation

Projects
None yet
2 participants
@farmboy0
Contributor

farmboy0 commented Aug 15, 2016

This part introduces special node structures to hold mesh and dangly data.
It also allows attaching models directly to other models as this fixes a bug with the current animation system.

@DrMcCoy

This comment has been minimized.

Show comment
Hide comment
@DrMcCoy

DrMcCoy Aug 17, 2016

Member

Hmm, for some reason, characters are now slightly transparent in your branch.

This is noticable, for example, on the hair of Bim in the NWN OC Prelude. And in Chapter 2, on, for example, Linu:

lilutransparent lilusolid

(Left is your branch, right xoreos/master)

It could be related to the environment mapping on her armor. Or maybe not, since it happens on heads, too (just not this noticable, for the most part).

Member

DrMcCoy commented Aug 17, 2016

Hmm, for some reason, characters are now slightly transparent in your branch.

This is noticable, for example, on the hair of Bim in the NWN OC Prelude. And in Chapter 2, on, for example, Linu:

lilutransparent lilusolid

(Left is your branch, right xoreos/master)

It could be related to the environment mapping on her armor. Or maybe not, since it happens on heads, too (just not this noticable, for the most part).

Show outdated Hide outdated src/graphics/aurora/modelnode.h
IndexBuffer indexBuffer; ///< Node geometry index buffer.
std::vector<TextureHandle> textures; ///< Textures.

This comment has been minimized.

@DrMcCoy

DrMcCoy Aug 17, 2016

Member

Trailing whitespace in lines 138 and 140

@DrMcCoy

DrMcCoy Aug 17, 2016

Member

Trailing whitespace in lines 138 and 140

@DrMcCoy

This comment has been minimized.

Show comment
Hide comment
@DrMcCoy

DrMcCoy Aug 17, 2016

Member

Apart from that tiny style issue in 32484c3 and that bigger transparency issue, the PR looks rather good. Good job! :)

Member

DrMcCoy commented Aug 17, 2016

Apart from that tiny style issue in 32484c3 and that bigger transparency issue, the PR looks rather good. Good job! :)

@DrMcCoy

This comment has been minimized.

Show comment
Hide comment
@DrMcCoy

DrMcCoy Aug 17, 2016

Member

Hmm, while checking your branch for leaks, I found something "interesting":

There's apparently at least two animations with the same name in at least one model. The model in question is "PLC_H09", the animation name is "die", and the model shows up in the entry area of the Chapter 1 of the NWN OC. I think it might be the mat on the ground where the infected lie, or maybe the infected themselves.

Here is a small patch against your branch that spits out a warning (and also for duplicate states). That also takes care of the memory leak, I think (because std::map::insert() won't overwrite a value, we're basically allocating the extra Animation and then throwing it away without delete'ing it; the patch just bails before allocating the Animation).

I'm not really sure how to handle this. Which of the two animations is the real one? Are the different, even? Does it matter?

(I probably should have added a check there earlier, too. I just didn't think that this could happen, but, well, BioWare. :P)

This is also not necessarily related to your PR as such, but somewhat connected. I'm mainly just thinking out loud here, because it might interest you and so I'm not forgetting this. And maybe someone wants to chime in and/or research this further. :)

Member

DrMcCoy commented Aug 17, 2016

Hmm, while checking your branch for leaks, I found something "interesting":

There's apparently at least two animations with the same name in at least one model. The model in question is "PLC_H09", the animation name is "die", and the model shows up in the entry area of the Chapter 1 of the NWN OC. I think it might be the mat on the ground where the infected lie, or maybe the infected themselves.

Here is a small patch against your branch that spits out a warning (and also for duplicate states). That also takes care of the memory leak, I think (because std::map::insert() won't overwrite a value, we're basically allocating the extra Animation and then throwing it away without delete'ing it; the patch just bails before allocating the Animation).

I'm not really sure how to handle this. Which of the two animations is the real one? Are the different, even? Does it matter?

(I probably should have added a check there earlier, too. I just didn't think that this could happen, but, well, BioWare. :P)

This is also not necessarily related to your PR as such, but somewhat connected. I'm mainly just thinking out loud here, because it might interest you and so I'm not forgetting this. And maybe someone wants to chime in and/or research this further. :)

@farmboy0

This comment has been minimized.

Show comment
Hide comment
@farmboy0

farmboy0 Aug 18, 2016

Contributor

I'll check it out and find a solution after comparing the values for both animations.
Btw one of the 2 checks should suffice because animations and states are the same thing in the models.

Contributor

farmboy0 commented Aug 18, 2016

I'll check it out and find a solution after comparing the values for both animations.
Btw one of the 2 checks should suffice because animations and states are the same thing in the models.

@DrMcCoy DrMcCoy merged commit c531a2a into xoreos:master Aug 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment