Skip to content

Fix ignore edge region. Center edge had wrong size and could have wrong coordinates - #375

Draft
atsju wants to merge 6 commits into
masterfrom
JST/374
Draft

Fix ignore edge region. Center edge had wrong size and could have wrong coordinates#375
atsju wants to merge 6 commits into
masterfrom
JST/374

Conversation

@atsju

@atsju atsju commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Somewhat improves #374. It does not fix all identified issues. What is fixed here:

  • inside circle is not always defined. So we use outside circle coordinates. They are concentric anyway.
  • insideOffset was added twice leading to wrong mask size

@atsju
atsju requested review from githubdoe and gr5 August 16, 2026 17:01
@github-actions

Copy link
Copy Markdown

🚀 New build available for commit 479c7cf
Download installer here

@gr5

gr5 commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Okay I have mixed feelings here. But in general I don't think I like this pr. You might not know this but you can move the inner outline when you are drawing the inner and outer circles on the igram:

image

You just draw the inner outline and then use the arrows to move it around. I don't think Dale ever intended this to work but... it works. I spent weeks fixing bugs when you average multiple of these wavefronts where the inner circles don't line up. Or are different diameters. Regions as well had issues with averaging but it's all fixed many versions ago.

So:

  1. We could get rid of this feature where you can move the inner diameter by an offset. I mean it's basically just another "region". It's different from the annulus feature even though in our heads it's closely related.
  2. Better: check to see if the inner region exists and is valid before using those center coordinates.
  3. Best?: Disable this feature ("Ignore Edge Region - Center Edge") when there is no inner outline

I don't understand what this is supposed to fix. If there is no inner outline, this feature doesn't seem to do anything. And if there is an inner outline it seems to work fine.

In the image below, the upper black circle is the inner outline feature and the lower black circle is from the mirror configuration dialog: "obstruction". This is all without even getting into the annulus feature!

image

I understand that there may also be a bug related to calculating annular nulls but this PR doesn't try to address that.

@atsju

atsju commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

OK I see.

You need to test 8.4.0 with a small igram (100pixels) to see the issue.
Center Edge has an effect but on coordinates -1/-1. Thus you will see it only on smaller igrams with larger center edge.
And it this case you will also see that the center edge has wrong dimension.

That being said, I confirm I didn't know the "central" hole could be moved. So I agree with your conclusions that this PR should not be merged like this. Because I specifically made assumption about the central hole being concentric.

I will probably go with solution 2 : check validity. I don't like solution 3 as masking center always makes sense.

@atsju
atsju marked this pull request as draft August 20, 2026 16:07
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