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

experimental cli export feature #1

Merged
merged 1 commit into from
Aug 14, 2019
Merged

Conversation

Gnumaru
Copy link
Contributor

@Gnumaru Gnumaru commented Aug 12, 2019

These changes implement an experimental cli export feature wich allows for selectively generating any number of the supported types of generated maps from a given diffuse texture (and optionally a given pressets file) and exiting without loading the main window.

Unrelated to the above mentioned feature, I made a bat script to simplify deploying on windows and added one line to gitignore to ignore the deploy directory

@azagaya
Copy link
Owner

azagaya commented Aug 12, 2019

Hi! thanks for the PR, i'll review in this days!
I've noticed you changes some operations in normal map calculation. I don't know why you got seg-faults as i compiled in various systems (cause my laptop is dead) and worked. Anyways, i'll test it all together!

@azagaya azagaya self-requested a review August 12, 2019 00:54
@azagaya azagaya added the enhancement New feature or request label Aug 12, 2019
@azagaya azagaya added this to the 1.6 milestone Aug 12, 2019
@Gnumaru
Copy link
Contributor Author

Gnumaru commented Aug 13, 2019

Sory, I forgot to mention the fix I needed to do on ImageProcessor::calculate_normal. I also thought those segfaults where strange because that code was the one used on the oficial builds, which where not giving any error for me, It just appeared in the build I compiled myself...

First I thought it should be due to some difference in the way I compiled opencv or some difference in our build setups...

Looking at Mat::at implementation on mat.inl.hpp we dont seem to have any kind of bounds check (it should be checking for px.x and pt.y values less than zero and greather than the limits of 'data'

return ((const _Tp*)(data + step.p[0] * pt.y))[pt.x];

It does a bunch of debug asserts before the return but none of them change the value of 'pt'...

One more thing. I just shamelesly copied the presset apply logic from pressetsmanager.cpp into main because I wanted to change the minimum ammount of files, but I know I should have refactored PresetsManager creating dedicated methods for applying the presets and then called those on main. Well, I should not really be calling it from main, but whe should have the command line parsing logic outside the main function (and preferably outside main.cpp), anywhere else, be it a class or a free function. But as I said before I was quickly implementing the bare minimum for my personal needs.

If you want I can refactor PresetsManager::on_pushButtonAplyPreset_clicked and extract the preset apply logic out of there and call it from main so that we don't need to have that awful code duplication.

@azagaya
Copy link
Owner

azagaya commented Aug 13, 2019

Hi! thanks for the extra info!

should be checking for px.x and pt.y values less than zero and greather than the limits of 'data'

As i'm using % operator, i think p.x and p.y shouldn't exceed that limits anyways, i think. Anyways, i must look at it closer, but i think if that is needed, i should add forward and backwards finite difference operators for edges of images. I'll see it later.

If you want I can refactor PresetsManager::on_pushButtonAplyPreset_clicked and extract the preset apply logic out of there and call it from main so that we don't need to have that awful code duplication.

That would be nice! i was thinking to, after reviewing and merging your PR, doing it myself, but if you want to give it a go, it will be very helpful!

One more thing, could you add some comments to your commits? all appearse with just a dot here.

Thanks a lot for your help! i really appreciate it!

@Gnumaru
Copy link
Contributor Author

Gnumaru commented Aug 13, 2019

The modulus operator sometimes return negative values. You can test it with this snippet of code:

printf('%d, %d, %d, %d\n', 1%2, (-1)%2, 1%(-2), (-1)%(-2));

It is also mentioned here
https://stackoverflow.com/questions/7594508/modulo-operator-with-negative-values

And sorry for the 'emtpy' commit messages. I'm terrified with the idea of losing even one single line of code so I usually commit and push all the time. Not just commit, because I could have some failure in my local machine and lose the local repository, so I commit and push all the time. Since these frequent commits are just fragments of something, I would end up using the same commit messages for every commit, like several commits in a row with the message 'continuing implementation of task 1234'. So, instead of repeating the messages, I throw up a bunch of dots. But I surely agree that several identical commit messages are bether than none, so I will stop with the dot thing.

@azagaya
Copy link
Owner

azagaya commented Aug 13, 2019

The modulus operator sometimes return negative values

I agree, but only if any argument is negative right? In this case, the for loops has always positive indexes, and the size is always positive, so modulo should be always positive... isn't that right?

So, instead of repeating the messages, I throw up a bunch of dots. But I surely agree that several identical commit messages are bether than none, so I will stop with the dot thing.

You don't need to stop doing that if that makes you feel comfortable.. just i will ask you to squash the commits before the Merge. We can continue working like this and at the end we squash them.

@azagaya
Copy link
Owner

azagaya commented Aug 13, 2019

Oh i see now! x-1 or y-1 can be negative... sorry i'll look into this!

@azagaya
Copy link
Owner

azagaya commented Aug 13, 2019

I will have to add foward finite difference operator for x and y equal to 0 and backwards to x and y ecual to with or height. It won't be difficult... Now i cannot understand why official build doesn't crash!

@azagaya
Copy link
Owner

azagaya commented Aug 13, 2019

@Gnumaru would you check if in your pc this code solves it?

    for(int x = 0; x < aux.cols; ++x)
    {
        for(int y = 0; y < aux.rows; ++y)
        {
            if (heightMap.at<Vec4b>(y,x)[3] == 0.0 ){
                normals.at<Vec3f>(y,x) = Vec3f(0,0,1);
                continue;
            }
            if (x == 0){
                dx = -3*aux.at<float>(y,x) + 4*aux.at<float>(y,x+1) - aux.at<float>(y,x+1);
            }else if (x == aux.cols-1){
                dx = 3*aux.at<float>(y,x) + 4*aux.at<float>(y,x-1) - aux.at<float>(y,x-2);
            }else{
                dx = -aux.at<float>(y,(x-1)%aux.cols) + aux.at<float>(y,(x+1)%aux.cols);
            }
            if (y == 0){
                dy = -3*aux.at<float>(y,x) + 4*aux.at<float>(y+1,x) - aux.at<float>(y+2,x);
            }else if (y == aux.rows-1){
                dy = 3*aux.at<float>(y,x) + 4*aux.at<float>(y-1,x) - aux.at<float>(y-2,x);
            }else{
                dy = -aux.at<float>(y-1,x) + aux.at<float>(y+1,x);
            }


            Vec3f n = Vec3f(-dx*(depth/1000.0)*normalInvertX,
                            dy*(depth/1000.0)*normalInvertY,
                            1*normalInvertZ);
            normals.at<Vec3f>(y,x) = n;

        }
    }

@Gnumaru
Copy link
Contributor Author

Gnumaru commented Aug 14, 2019

Yes, I can confirm this solves code solves the issue.

But I noted that the segfault only happens when I try to export normal maps from the command line, not when I open up the application window.

At first It seemed that, when running from the main window, the code at the start of the inner loop

if (heightMap.at<Vec4b>(y,x)[3] == 0.0 ){
	normals.at<Vec3f>(y,x) = Vec3f(0,0,1);
	continue;
}

never let the program reach the lines bellow with x and y values equal to zero. This happens when oppening the program from the first time, the initial x and y values when reaching "dx = -aux.at" etc are x:38 and y:92 every time I open up the program.

But when I load a new image I can reach the dx and dy lines with x:0 and y:0 and, even so, the program wont cause a segfault with the old code... That's just weird...

@Gnumaru
Copy link
Contributor Author

Gnumaru commented Aug 14, 2019

The else for your x==0 is identical to the old code

dx = -aux.at<float>(y,(x-1)%aux.cols) + aux.at<float>(y,(x+1)%aux.cols);

but the else for your y==0 is different

dy = -aux.at<float>(y-1,x) + aux.at<float>(y+1,x);

Is that intentional?

@azagaya
Copy link
Owner

azagaya commented Aug 14, 2019

Hi!

At first It seemed that, when running from the main window, the code at the start of the inner loop..

Well, that line would actually prevent images with transparent pixels in the borders to crash.. that may be a cause of me not beeing able to reproduce the problem. But i've tried with full opaque sprites (there are screenshots on itchio) so idk. But surely we must change that.

Is that intentional?

No, i missed that. Modulo operation is no longer needed.

@azagaya
Copy link
Owner

azagaya commented Aug 14, 2019

Hey!
I've tested (about time) your code and works great to me!

One more thing. I just shamelesly copied the presset apply logic from pressetsmanager.cpp into main because I wanted to change the minimum ammount of files, but I know I should have refactored PresetsManager creating dedicated methods for applying the presets and then called those on main. Well, I should not really be calling it from main, but whe should have the command line parsing logic outside the main function (and preferably outside main.cpp), anywhere else, be it a class or a free function. But as I said before I was quickly implementing the bare minimum for my personal needs.

If you wish, i can merge it as is, so we can release it for the 1.5.1 bug fix (to also solve the issues reported). Later we can make it nicer with the comments you said before. The only thing i will ask you to do, please, is to squash all the commits and add some comments, so we can easily find changes later. Would you?

@Gnumaru Gnumaru force-pushed the master branch 4 times, most recently from 359fccf to fed57df Compare August 14, 2019 21:56
@azagaya azagaya modified the milestones: 1.6, 1.5.1 Aug 14, 2019
@azagaya azagaya merged commit 62bbf61 into azagaya:master Aug 14, 2019
@azagaya
Copy link
Owner

azagaya commented Aug 14, 2019

Thanks a lot!

@Gnumaru
Copy link
Contributor Author

Gnumaru commented Aug 14, 2019

The commits are squashed now. It took a while because I never squashed commits before, but now it should be one single commit with a significant message.

I removed the bat script I created for deploying to keep the changes to a minimum. I didn't remove the changes to the readme because I think knowing how to compile on windows makes it easier to grab contributors to the project.

@Gnumaru
Copy link
Contributor Author

Gnumaru commented Aug 14, 2019

Thanks a lot!

You're welcome =)

@azagaya
Copy link
Owner

azagaya commented Aug 14, 2019

Also fixes #3

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

Successfully merging this pull request may close these issues.

2 participants