-
Notifications
You must be signed in to change notification settings - Fork 248
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
[Core] Controllers - Add Output Controller #11509
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that thes changes in the base controller really make sense. My unique comment is the new DefaultController
, why is it required?
EDIT: I'd take the chance to also update the GiD output process accordingly.
@rubenzorrilla Removed the I will change the gid_output process as well. |
I agree with the changes in the base class, it makes complete sense to separate it into two methods 👍 |
mModelPartName = Settings["model_part_name"].GetString(); | ||
|
||
const auto& output_control_type = Settings["output_control_type"].GetString(); | ||
if (output_control_type == "step") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with the changes, but tbh, if we are going to derive different controllers from the base, I would move this to a different one (Time for time, StepController for steps, etc...).
But I would understand leaving this for historical reasons...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is for the historical reasons :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the same. Maybe we can call it OutputController to make clear that this is for the output and save the Time keyword for an eventual time-based cotroller (same argument applies to the step keyword).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. I will do it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done :) can somebody approve this?
Can someone approve this? @rubenzorrilla @roigcarlo @ddiezrod ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the implementation.
📝 Description
Currently, the
Controller::Evaluate
method is made non-const which makes it to have multiple responsibilities such as evaluating a given controller and returning a bool, and changing the state of the controller. This becomes problematic if someone wants to evaluate multiple times in the same state which is not possible without hackish ways. In this PR, I would like to seperate these two responsibilities, so one can evaluate multiple times in the same state, and if required to change state, they can do it seperately.Controller::Evaluate
method is madeconst
so that, now one can only do evaluation, not changing the state of the controller.Controller::Update
method is introduced which allows changing the state of theController
.I introduced controllers:
OutputController
-> This has the exact same logic which is used inVtkOutputProcess
andVtuOutputProcess
(Which will be used inHDF5OutputProcess
as well.TODO
🆕 Changelog
OutputController
VtuOutputProcess
,VtkOutputProcess
,GidOutputProcess
,UnvOutputProcess