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

Changed ktSector Operation to compute divisions for sectors. #177

Merged
merged 2 commits into from
May 28, 2024
Merged

Changed ktSector Operation to compute divisions for sectors. #177

merged 2 commits into from
May 28, 2024

Conversation

sganz
Copy link
Contributor

@sganz sganz commented May 28, 2024

Changed ktSector Operation to compute divisions for sectors. Removed SectorDivisions property. Some general clean up.

…SectorDivisions property. Some general clean up.
Copy link
Contributor Author

@sganz sganz left a comment

Choose a reason for hiding this comment

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

Updating the knob to v2.10. Fixed up sector operations

Copy link
Contributor Author

@sganz sganz left a comment

Choose a reason for hiding this comment

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

Approved

@lainz lainz merged commit f11b361 into bgrabitmap:dev-bgracontrols May 28, 2024
@circular17
Copy link
Contributor

circular17 commented May 28, 2024

Hi @sganz

From what I gather, the code you've used wasn't the latest as the check on FPC_FULLVERSION was reverted and some code moved. I'll fix that. the takeaway here is not to approve to quickly your changes and rather wait for someone else, me or Lainz to have quick look.

@lainz
Copy link
Member

lainz commented May 28, 2024

Hi. A good solution is to delete the branch and work in dev-bgracontrols directly. I think you have access.

@circular17
Copy link
Contributor

Delete which branch?

@lainz
Copy link
Member

lainz commented May 28, 2024

The branch on his/her github account.

@sganz
Copy link
Contributor Author

sganz commented May 28, 2024

@circular17 I messed up the commit, and likely went back to the version that had the wrong code for the compiler option, manual code update mistake, but I think I was on the current branch except for my handy work on this one.

@lainz yes, I think that is a good idea. I forked it so when I made github mistakes they would initially only mess with my repo until I got them sorted out 😢

I need to be more careful and just stop instead of trying to fix stuff up, which in my case always seems to make it worse.

Hopefully the code is good.

Sandy

@circular17
Copy link
Contributor

Hi Leandro,

The branch on his/her github account.

Ah indeed updating a fork is not always obvious to do.

Hi Sandy,

Yes, sometimes starting fresh is the simplest option. It can take time to change our habits.

I've reviewed the changes and added your beautiful test program. It all works fine and I've made slight adjustements so that changing the knob type doesn't reset the value:.

I've added default values for properties so that they are not written in the LFM file.

Thinking about it, the sector division doesn't seem necessary. The sector span is always equal to 1. So CalcSectorFromValue could simply round the value, or am I missing something? And CalcValueFromSector is in fact not necessary. What do you think?

Regards

@sganz
Copy link
Contributor Author

sganz commented May 28, 2024

I deleted my forked bgracontrols repo, hopefully that will be a better way to go. Let's see how that works out...

And yes, in looking at the CalSectorFromValue and CalcValueFromSector both seem like can be removed or cleaned up as sectorSpan will always be 1. I need to check to see if any of the range limit checks in CalcValueFromSector are really needed, as that may be the only thing, but have to take a look. Good catch, simpler is better! Will do some more coding tonight and see if all good, and then attempt to not botch up the git work :)

No problem changing anything in the tester program, glad it's useful, was helpful for me doing a lot of simple and repetitive testing.

Sandy

@circular17
Copy link
Contributor

Yes, it is practical to have a test program to play with the components!

I've reviewed the new changes, and they look perfect. The modifications are clear and easy to distinguish. Thanks for the efforts, Sandy!

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.

3 participants