Skip to content

Fixes issue #115 - issues with "edge mask" aka aperture reduction - #116

Merged
gr5 merged 1 commit into
masterfrom
issue115
Nov 23, 2023
Merged

Fixes issue #115 - issues with "edge mask" aka aperture reduction#116
gr5 merged 1 commit into
masterfrom
issue115

Conversation

@gr5

@gr5gr5 commented Nov 22, 2023

Copy link
Copy Markdown
Collaborator

When reading old OLN files, ignore edge mask
When writing OLN files don't save edge mask
Fixed 2 locations where edge mask needs to be set to zero when checkbox is not checked in mirror dialog Fixed bug where edge mask is checked if not found in OLN file!

When reading old OLN files, ignore edge mask
When writing OLN files don't save edge mask
Fixed 2 locations where edge mask needs to be set to zero when checkbox is not checked in mirror dialog
Fixed bug where edge mask is checked if not found in OLN file!
Comment threadigramarea.cpp
}
}
else{ // just enable edge mask check box to use the current value.
md.changeEdgeMaskvalues(md.aperatureReduction);

@gr5gr5Nov 22, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this line of code (line 2180 just above) is particularly nasty and found when reading OLN file: both the older V6 style OLN file and also when reading V7. It checks the "edge mask" checkbox even if nothing is found regarding edge mask in the OLN file!

Not only did I get rid of the above code in both "writeOLN" functions, I got rid of the entire function (which is where it checks the box in the gui). That function, changeEdgeMaskvalue(), was used in many places when reading OLN files but nowhere else. So I got rid of it.

@gr5

gr5 commented Nov 22, 2023

Copy link
Copy Markdown
CollaboratorAuthor

I spent a lot of time testing all the changes. It seems to work fine now.

@gr5gr5 assigned gr5 and githubdoe and unassigned gr5Nov 22, 2023

@githubdoegithubdoe left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looks like it was pretty messed up with a lot of omissions. Makes me wonder what I was thinking. Guess I was distracted. Did not realize all the implications. Still puzzled why I did so much work in the reading and writing of it in the .oln file. That worries me that I thought it necessary. Now of course I think it was not.

@gr5
gr5 merged commit d6f8508 into masterNov 23, 2023
@gr5

gr5 commented Nov 23, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Dale, Do you want me to release 7.2 now just for this change? It's not too much trouble. I need to test the final release if I do a release it but I'll probably just do a few quick tests (load igram, process) as not much changed. And I already tested the changes. Although I will need to update the revision history and you'll need to approve that.

@githubdoe

Copy link
Copy Markdown
Owner

I think it would be nice but I will leave it up to you to decide based on your schedule and such.

Comment threadigramarea.cpp
@@ -2257,40 +2220,7 @@ void IgramArea::loadOutlineFileOldV6(QString fileName){
mirrorDlg &md = *mirrorDlg::get_Instance();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can be deleted as md is not used anymore. This will fix the [-Wunused-variable]

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. Already merged. Didn't see your comment until now.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can fix in the other PR. It's really minor.
This is how dead code comes to live 👻

@atsju

Copy link
Copy Markdown
Collaborator

Dale, Do you want me to release 7.2 now just for this change? It's not too much trouble. I need to test the final release if I do a release it but I'll probably just do a few quick tests (load igram, process) as not much changed. And I already tested the changes. Although I will need to update the revision history and you'll need to approve that.

I wonder if this should be v7.1.2. This would be a pure bugfix release.
Sorry if this seems futile and great work to find and fix this. Thank you

Sign up for freeto 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.

3 participants

@gr5@githubdoe@atsju