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

Generate dynamic images #1159

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

NikEfth
Copy link
Collaborator

@NikEfth NikEfth commented Feb 14, 2023

Added the simple ability to generate dynamic images based on a .fdef file.
The modifications are simple and backward compatibility is maintained.

Use shared_ptr to take single frames from the DynamicDiscretisedDensity.
@NikEfth
Copy link
Collaborator Author

NikEfth commented Feb 14, 2023

If there is interest, I have an additional class that can calculate simple types of motion (rotation for now) and refresh the shapes in each time frame.

In the frame comparison we need to subtract 1 to be consistent
@NikEfth NikEfth self-assigned this Feb 16, 2023
Copy link
Collaborator

@robbietuk robbietuk left a comment

Choose a reason for hiding this comment

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

I like it. Just a few comments or concerns.

examples/ROOT_files/ROOT_STIR_consistency/SourceFiles/.generate_image1.par.swp is a binary file, is it needed?

Comment on lines -303 to +352
get_output_sptr()
get_output_sptr(unsigned int frame)
{
return out_density_ptr;
return out_density_ptr->get_density_sptr(frame);
}

shared_ptr<DynamicDiscretisedDensity>
GenerateImage::
get_all_outputs_sptr()
{
return out_density_ptr;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not keep get_output_sptr() with no arg and add get_output_sptr(unsigned int frame) method? Maybe this is clearer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be, or actually we wouldn't really need get_output_sptr(frame) really. However, sadly I cannot see a way to preserve backwards compatibility.

@@ -134,7 +141,9 @@ class GenerateImage : public KeyParser
Succeeded save_image();

//! Returns the discretised density with computed shapes.
shared_ptr<DiscretisedDensity<3, float>> get_output_sptr();
shared_ptr<DiscretisedDensity<3, float>> get_output_sptr(unsigned int frame = 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like this default might cause problems later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When do you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, bad comment. I think I meant it doesn't preserve backways compatability.

@KrisThielemans
Copy link
Collaborator

If there is interest, I have an additional class that can calculate simple types of motion (rotation for now) and refresh the shapes in each time frame.

It would be of interest, but it'd have to work with the motion stuff in experimental really (and we'd have to promote that).

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.

Interesting and small. I'm not so sure I like adding frames to the Shape3D class. It seems frames don't have too much to do with a shape.

Maybe there's a way to change the value keyword into a TAC? Might be less obvious, but would be quite nice...

Comment on lines +248 to +252
out_density_ptr.reset(new DynamicDiscretisedDensity(exam_info_sptr->get_time_frame_definitions(),
rel_start_time,
scn,
tmpl_image) );

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm astonished that we don't have a constructor for DynamicDiscretisedDensity that takes an ExamInfo. That would be far better. Maybe add it in this PR?

@@ -143,6 +145,9 @@ class DynamicDiscretisedDensity: public ExamData
const singleDiscDensT &
get_density(const unsigned int frame_num) const ;

shared_ptr<DiscretisedDensity<3, float> >
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think would need to return shared_ptr<const singleDiscDensT> : correct template but also prevents modifying the underlying object.

@KrisThielemans
Copy link
Collaborator

Let's do that for STIR 6.0, i.e. after release of 5.2, as then we can break backwards compatibility as much as we like!

Anyone any ideas on adding a TAC to shapes? Ideally would include integration of the TAC for a time-frame. Possibly we can re-use some of the KineticModel stuff.

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