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

Modify for event_integration #2

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chenyx512
Copy link

No description provided.

}
if (event_frame_msg.data[pos] < 255) // prevent overflow
event_frame_msg.data[pos]++; // feed this into model
// event_frame_msg.data[pos] = 255 // use this for display purpose (visualize)
Copy link
Author

Choose a reason for hiding this comment

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

@aamini use this line if you want to visualize the event input at the same time. This doesn't influence the event+RGB model, but it may mess up the old event model that takes in normalized event_frames rather than one-hot event_frames.

Copy link
Member

@igilitschenski igilitschenski left a comment

Choose a reason for hiding this comment

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

All in all, this looks great to me. Is it worth making it such that integration becomes a configurable option? Then we could open a pr to upstream later on.

{
event_array_msg->header.stamp = e.ts;
}
if (event_frame_msg.data[pos] < 255) // prevent overflow
Copy link
Member

Choose a reason for hiding this comment

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

Should there be an else statement with some warning in case of Overflow happening.

Comment on lines +682 to +685
int e_x = caerPolarityEventGetX(event);
int e_y = caerPolarityEventGetY(event);
bool e_polarity = caerPolarityEventGetPolarity(event);
int pos = e_y * event_frame_msg.step + e_x * 3 + e_polarity;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move the variable declarations outside of the loop to the beginning of the method? Not sure if it will make a big difference as the compiler should be smart enough to optimize.

@chenyx512
Copy link
Author

All in all, this looks great to me. Is it worth making it such that integration becomes a configurable option? Then we could open a pr to upstream later on.

Considering the amount of work they did with DAVIS, I believe they should have thought about this. Their dvs_renderer package (https://github.com/mit-drl/rpg_dvs_ros/tree/master/dvs_renderer) subscribes to events and output integrated frames ready for display. However:

  1. I am not familiar with ROS so I am not sure about whether having two nodes could have an impact on speed due to the ROS traffic.
  2. I have tried reading their code (for a short while), but apparently they do more than our simple integration in their code (such as config for 1-channel or 2-channel integration), and with almost no documentation, it is not easy to understand...

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.

2 participants