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

changing SPECTUB global variables to class members #1169

Merged
merged 5 commits into from
Apr 19, 2023

Conversation

danieldeidda
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

remove comment in

\todo Variables wm, wmh and Rrad are currently global variables. This means that this code would be very dangerous

remove extern declarations at

extern wm_da_type wm; //! weight (or probability) matrix
extern wmh_type wmh; //! information to construct wm
extern float * Rrad; //! radii per view

Will need to disable the copy-constructor which is something like

// disable copy-constructor as currently unsafe to copy due to bare pointers
ProjMatrixSPECTUB(const &ProjMatrixSPECTUB) = delete;

Ideally (in next step) make Rrad and std::vector such that we don't need to deallocate etc. Can then be passed to SPECTUB functions as &Rrad[0], which is of course a bit ugly... So we can leave that for later.

@danieldeidda
Copy link
Collaborator Author

Will need to disable the copy-constructor which is something like

// disable copy-constructor as currently unsafe to copy due to bare pointers
ProjMatrixSPECTUB(const &ProjMatrixSPECTUB) = delete;

Where should this go I am guessing in the ProjMatrixByBinSPECTUB.h?

@KrisThielemans
Copy link
Collaborator

notice a syntax error. that'll likely have to be

ProjMatrixSPECTUB(const ProjMatrixSPECTUB&) = delete;

removing unnecessary namespace;
@KrisThielemans
Copy link
Collaborator

let's try to make float * Rrad a const float * Rrad where possible

@KrisThielemans
Copy link
Collaborator

Linking errors

SPECTUB_Tools.cxx:(.text+0x3b0a): undefined reference to `SPECTUB::wmh

so some functions are still expecting the global varaibles

@KrisThielemans KrisThielemans changed the title making useful global variable class members changing SPECTUB global variables to class members Apr 17, 2023
@KrisThielemans KrisThielemans linked an issue Apr 18, 2023 that may be closed by this pull request
Copy link
Collaborator

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

Great! It is working fine.

Can you clean-up by removing comments, and adding something to release_5.2.htm? (along the lines of "removed global variables in the SPECTUB code, such that we can now have multiple SPECTUB projectors (PR #1169)"

@KrisThielemans
Copy link
Collaborator

@samdporter want to try this out? it should work now.

@KrisThielemans
Copy link
Collaborator

@mastergari we'll have to do the same for the pinhole code presumably. can do that later.

@samdporter
Copy link
Contributor

@samdporter want to try this out? it should work now.

Looks good with the tests in here.

I'll do something a bit more difficult and let you know

Sam

Copy link
Collaborator

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

great. @samdporter let us know when you're happy.

Ok to squash-merge?

@samdporter
Copy link
Contributor

samdporter commented Apr 19, 2023

great. @samdporter let us know when you're happy.

Ok to squash-merge?

Looks good to me

see updated repo

@KrisThielemans KrisThielemans linked an issue Apr 19, 2023 that may be closed by this pull request
@KrisThielemans KrisThielemans merged commit c0cc3a2 into UCL:master Apr 19, 2023
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.

Multiple projectors with SPECTUBMatrix cause double free error SPECTUB code uses global variables
3 participants