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

WIP/RFC: normalmaps #1

Open
wants to merge 7 commits into
base: make-nodes
Choose a base branch
from

Conversation

mjtorn
Copy link

@mjtorn mjtorn commented Feb 28, 2019

As I suggested in EsotericSoftware/spine-runtimes#728 I have spent a bit of time trying to get normal maps even to load.

My initial test was making a new SpineResource for it, but decided that with the resource getting grafted all over the Spine node, I would rather make the loading of normal maps (at least for the time being) as automagic as possible.

This code segfaults pretty much every time. I have had the occasional luck that I can execute the editor from VS Code, but it would still segfault when running the project.

Going forward with the illumination/shading, I'll have to hear it from someone or figure it out. I'm thinking I'd have to keep the regular and normal-mapped animations in sync and find some way of having Godot look at the normal maps from the normal-mapped animation. It would be more elegant if the Texture slots would have a normal map from the second atlas, but I don't think it's possible except for Sprites.

Can the code be altered to use Sprite? With any luck – though I haven't had much yet! – Godot would Do The Right Thing if that was the case.


I don't have an example project that I can share publicly, our assets are our own, but hopefully I can find an example at some point.

You should just have a copy of your atlas with the prefix nm_ where your spine atlas is, and have it point to a normal map


Comments and clues are welcome, as I don't know how much time I have for this, and chasing down segfaults that don't make any apparent sense is one of my least favorite activities.

@chanon
Copy link
Owner

chanon commented Feb 28, 2019

Wow, well I never worked with normal maps. Just tried reading about them a bit, but still no idea about how to use them in Spine.

I will probably need some example assets to be able to help.

Reviewing the code, looks like you're just at the point of trying to get them to load. I might be able to help debug that.

Then for drawing, if you look at this commit:
3c8f398

You will see the main change I made in my fork compared to original. (I had to look at it just now too cause I completely forgot what I did.)

In the original they just looped over each skeleton slot and drew them:
3c8f398#diff-3968a79d03606e0441798992e28936c9L168

While in my version, I created "SpineSlot" nodes to handle the drawing of each skeleton slot instead:
3c8f398#diff-3968a79d03606e0441798992e28936c9R666 < I create the SpineSlot nodes
3c8f398#diff-3968a79d03606e0441798992e28936c9R177 < I'm just using the same old code to draw

You probably know this as you added some code where I created the SpineSlots. You seem to be adding another set of SpineSlots for the normal maps .. which while I'm not really sure ... I think is not the right way to do this.

After looking at: https://jarlowrey.com/blog/2d-normal-godot.html it looks like the normal maps can be applied to a shader param in a material, which can be set on CanvasItems.

SpineSlots are actually extended fron Node2D
3c8f398#diff-8facda4c7f393bbf6f5ae382afe23409R5

And Node2Ds are CanvasItems. So I think you just need to use the textures in the loaded normal maps and apply them to the Normal shader param in a material and set it (or them?) to be the SpineSlots' materials.

@mjtorn
Copy link
Author

mjtorn commented Feb 28, 2019

I'll get back to this as soon as I can, but I'll reply a bit first ;)

Reviewing the code, looks like you're just at the point of trying to get them to load. I might be able to help debug that.
[...]
You seem to be adding another set of SpineSlots for the normal maps .. which while I'm not really sure ... I think is not the right way to do this.

I really need to find an example I can use and share. I agree that creating new slots is probably incorrect, but I was hoping to see something. I already threw one approach out the window (having a new resource) so I just committed what I had :D Considering how stuck I am with the initial segfault, that part is something to look at later.

So I think you just need to use the textures in the loaded normal maps and apply them to the normal map shader param in a material and set it (or them?) to be the SpineSlots' materials.

This would be ideal, I think, but I need to understand how to implement it. Once the engine doesn't segfault on me, I would have the normalmap textures somewhere and should be able to match them to the other slots, even without creating new slots for them, maybe.

Actually I think I should have dug deeper into the documentation, though I blame the segfaults for keeping me away, but it seems canvas_item_add_triangle_array takes RID normal_map as the last parameter.

If I'm understanding this correctly, I probably don't even need a custom canvas material and shader script, right?

Thanks and sorry for not having more to go with for now; hope I recover from the flu and other work real soon :D

@chanon
Copy link
Owner

chanon commented Feb 28, 2019

Actually I think I should have dug deeper into the documentation, though I blame the segfaults for keeping me away, but it seems canvas_item_add_triangle_array takes RID normal_map as the last parameter.

If I'm understanding this correctly, I probably don't even need a custom canvas material and shader script, right?

Possibly .. I don't know, but it looks like that might work. Yes, that would be even easier. (And might actually be the right way to do it ... not sure)

@mjtorn
Copy link
Author

mjtorn commented Feb 28, 2019

@chanon btw, created a repo with assets I got off the Unity tree https://github.com/mjtorn/godot-stretchyman-test - but I haven't seen that guy normal-mapped because I don't have Unity at the moment.

@mjtorn
Copy link
Author

mjtorn commented Feb 28, 2019

Please let me know if you have any luck with getting it to not segfault, and then we can figure out where to actually keep the normal map.

I mean, they have to be somewhere, and if the Spine animation deforms them, we have to get the deformed normal map back from Spine, right?

Thanks!

@mjtorn
Copy link
Author

mjtorn commented Mar 4, 2019

@chanon have you by any chance had an opportunity to look at this?

I was thinking about an alternative approach, but would like to hear comments on it, as I might have time tomorrow to look at this again for an hour or few.

It might be possible to load the normal-map PNG as a Texture into the Spine resource. It should issue an error if the resource isn't properly initialized and otherwise set the normal map states correct.

The thing I don't know about, at least for now, is how to re-use the existing atlas for that purpose. Considering the atlases need to be identical anyway, it might make life easier to have only one atlas and two atlas PNGs. Usability-wise this would be a bit like using Sprite by setting a separate normal map in there.

Food for thought.

@mjtorn mjtorn force-pushed the gh-mjtorn-chanon-normalmaps branch from e7c27e3 to 538e32d Compare March 16, 2019 12:24
@mjtorn
Copy link
Author

mjtorn commented Mar 16, 2019

I snuck some computer time away from the family and got something done. Please update your copy of https://github.com/mjtorn/godot-stretchyman-test and test the force-pushed code, it's very promising!

I'm not at all sure why 191a4e0 has to be the way it is, maybe I missed a spot where the pointer needs to be NULLed manually so it may have issues.

Help and testing still welcome, thanks! :)

@mjtorn mjtorn force-pushed the gh-mjtorn-chanon-normalmaps branch from 8afc811 to afe4773 Compare March 29, 2019 09:27
@mjtorn
Copy link
Author

mjtorn commented Mar 29, 2019

@chanon I had to drop the 3.0 compatibility commits. I have backups of them, if needed.

Problem was that the VERSION_MAJOR macros weren't defined. I don't know why or how, but I was just assuming VS Code dropped them and #defined them manually to get the compilation going. Turns out this was a real issue and it probably never worked.

Now that 3.1 is out, I think it's safe to support that and if someone wants this for 3.0, he or she can add a proper version check.

Can you confirm this works for you?

@chanon
Copy link
Owner

chanon commented Mar 29, 2019

I haven't had the time to review this yet, sorry! Maybe this week I will.

@mjtorn
Copy link
Author

mjtorn commented Mar 29, 2019

Ok, thanks!

For what it's worth, I built a clean editor (Linux) off the newly updated https://github.com/mjtorn/godot/tree/branch.3.1-stable.spine which has https://github.com/mjtorn/spine/tree/gh-mjtorn-chanon-normalmaps as its Spine module.

Then I ran the https://github.com/mjtorn/godot-stretchyman-test project (converted to 3.1).

Everything checked out!

Maybe next week I can do further testing with our proper game assets and see if lighting z-indexes can affect the normal-mapped characters. They should, but I'm not convinced they do.

@mjtorn
Copy link
Author

mjtorn commented Apr 1, 2019

Not sure it's about normal maps, but including this PR code does mess up game exports :(

I'll leave this here just in case, http://paste.debian.net/1075221/. Does look like whenever the module is included, exported games either segfault or abort.

I reached this conclusion because both the official Godot 3.1 download and a compiled one do export games that exit cleanly.

@mjtorn
Copy link
Author

mjtorn commented Apr 3, 2019

I pasted these links to the irc channel, but I'll leave them here too.

Our game-in-development systematically dies on "pure virtual method called", and I haven't sen a segfault there in a while. Weird thing is that it fails on the same thing even when quitting the game immediately from the main menu, which does not have Spine nodes! http://paste.debian.net/1075754/

The stretchyman test works better, but it does occasionally segfault (I have not seen any pure virtuals there), and the stack trace looks remarkably similar. http://paste.debian.net/1075753/

So this is not complete yet :( </3

@mjtorn mjtorn force-pushed the gh-mjtorn-chanon-normalmaps branch from 78d3002 to c82d8eb Compare April 4, 2019 12:50
@mjtorn
Copy link
Author

mjtorn commented Apr 5, 2019

@chanon That commit c78b90e replaces the extra check that prevented segfaulting. In this case it was a stupid oversight of mine where it simply wasn't inited as NULL.

Caught my eye while trying to solve the exported games crashing-when-quitting. That issue is still more than relevant, because it was not fixed in force-pushing.

I get crashes even with older commits... Can you confirm or deny that any of this works for you on 3.1? Does that stretchyman demo work? Do you have something public to test with for comparing notes?

Thanks!

@mjtorn
Copy link
Author

mjtorn commented Apr 10, 2019

"Backported" to 3.0 and the Stretchyman test seems to work reliably. Our player character not so much. It might be related to having multiple slots or something, but compared to a sprite it looks wrong :(

I think it would be best if this could be made to work on 3.1, if someone knew how to help out with it, so there'd be a more modern platform to fix other issues on. I mean, I had to hack our game a bit to make it run on 3.0 again, and going all the way down that path might not be any fun...

@mjtorn
Copy link
Author

mjtorn commented May 2, 2019

I'll leave the 3.0 tree here just in case.

https://github.com/mjtorn/spine/tree/gh-mjtorn-chanon-normalmaps-3.0

Haven't figured out why the player character and some others don't really work with this code... We have a simple item that does, far simpler than Stretchyman. Official support would be really, really welcome...

scottkunkel added a commit to scottkunkel/spine that referenced this pull request Jun 17, 2019
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.

2 participants