Skip to content

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.


//! 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?

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