Conners issue 58 saccade work #75

ManOfNeptune27 · 2025-09-17T19:10:01Z

No description provided.

There was a problem with merging that needed to be fixed.

gemini-code-assist

Summary of Changes

Hello @ManOfNeptune27, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the eye-tracking analysis capabilities by integrating comprehensive support for saccade detection and data management. It establishes a new data structure for saccades, extends the core algorithm framework to generate these events, and enables their storage within the database, providing a more complete picture of eye movement patterns.

Highlights

Saccade Data Structure: Introduced a new Saccade class to represent saccadic eye movements, including properties like start/end coordinates, amplitude, peak velocity, and average velocity.
Saccade Generation Interface: Extended the FixationAlgorithm interface with virtual methods (generateSaccades, getSaccades, computeSaccadeEstimate) to support the generation and retrieval of saccade data across different algorithms.
Algorithm Implementations: Implemented saccade generation logic within BasicAlgorithm, IDTAlgorithm, and IVTAlgorithm, allowing these algorithms to detect and calculate properties for saccades.
Database Integration: Enabled database persistence for saccade data by uncommenting and implementing insertSaccade and insertSaccadeGaze functions in the Database class, allowing saccade events and their associated gaze points to be stored.
Controller Updates: Modified the Controller to handle session_saccades, indicating that saccade data will now be processed and managed alongside fixation data.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces the initial framework for saccade detection and processing. Several new classes and methods related to saccades have been added, and existing database functions have been enabled.

While the changes lay a good foundation, there are several critical issues that need to be addressed:

There are multiple instances of SQL injection vulnerabilities in database.cpp which must be fixed by using prepared statements.
A critical bug in ivtalgorithm.cpp in calculateAverageVelocity will cause it to always return 0.
A copy-paste error in fixation.cpp leads to incorrect calculation of right_pupil_diameter.

Additionally, there are some design and maintainability concerns:

The database handle in database.h has been made public, breaking encapsulation.
There is significant code duplication between the Fixation and Saccade classes, which could be refactored by using a common base class.
Several functions contain bugs like potential division-by-zero or use of uninitialized variables.

Please review the detailed comments for specific suggestions on how to address these issues.

gemini-code-assist · 2025-09-17T19:14:02Z

database.cpp

+void Database::insertSaccade(QString saccade_id, QString fixation_run_id, QString start_time, QString end_time,
+                             QString start_fixation, QString end_fixation, QString start_x, QString start_y,
+                             QString end_x, QString end_y, QString amplitude, QString peak_velocity,
+                             QString average_velocity, QString direction, QString duration){
+    QString query=QString("INSERT INTO saccade(saccade_id,fixation_run_id,start_time,end_time,start_fixation,end_fixation,start_x,start_y,end_x,end_y,amplitude,peak_velocity,average_velocity,direction,duration) VALUES(%1,%2,%3,%4,\"%5\",\"%6\",%7,%8,%9,%10,%11,%12,%13,%14,%15);").arg(saccade_id,fixation_run_id,start_time,end_time,start_fixation,end_fixation,start_x,start_y,end_x,end_y,amplitude,peak_velocity,average_velocity,direction,duration);
+    sqlite3_exec(db,query.toStdString().c_str(),NULL,0,NULL);
+}


This method of constructing SQL queries by string formatting is vulnerable to SQL injection. An attacker could provide malicious input for parameters like start_fixation to execute arbitrary SQL commands. Please use prepared statements with parameter binding to prevent this. For example, with sqlite3, you would use sqlite3_prepare_v2, sqlite3_bind_*, and sqlite3_step. This vulnerability also exists in insertSaccadeGaze and other insert methods in this file.

gemini-code-assist · 2025-09-17T19:14:02Z

fixation.cpp

+
+
+        left_pupil_diameter += isnan(gaze.left_pupil_diameter) || gaze.left_pupil_diameter == -1.0 ? 0 : gaze.left_pupil_diameter;
+        right_pupil_diameter += isnan(gaze.right_pupil_diameter) || gaze.left_pupil_diameter == -1.0 ? 0 : gaze.right_pupil_diameter;


There appears to be a copy-paste error here. When calculating right_pupil_diameter, the condition checks gaze.left_pupil_diameter == -1.0 instead of gaze.right_pupil_diameter == -1.0. This will lead to incorrect calculations for the right pupil diameter.

Suggested change

right_pupil_diameter += isnan(gaze.right_pupil_diameter) || gaze.left_pupil_diameter == -1.0 ? 0 : gaze.right_pupil_diameter;

right_pupil_diameter += isnan(gaze.right_pupil_diameter) || gaze.right_pupil_diameter == -1.0 ? 0 : gaze.right_pupil_diameter;

gemini-code-assist · 2025-09-17T19:14:02Z

ivtalgorithm.cpp

+    for (int i=1;i>gazes.size();i++){
+        totalVelocity+=calculateGazeVelocity(gazes[i-1].x,gazes[i-1].y, gazes[i].x, gazes[i].y);
+    }


The loop condition i > gazes.size() is incorrect. Since i starts at 1, this loop will never execute. As a result, totalVelocity will always be 0, and calculateAverageVelocity will return 0 (or NaN if gazes.size() is 0). The condition should likely be i < gazes.size().

for (int i=1; i < gazes.size(); i++){ totalVelocity+=calculateGazeVelocity(gazes[i-1].x,gazes[i-1].y, gazes[i].x, gazes[i].y); }

gemini-code-assist · 2025-09-17T19:14:02Z

database.h

+    sqlite3* db;

 private:
-    sqlite3* db;
    QString file_path;
    bool open = false;


The database handle db has been moved from private to public. This breaks encapsulation and is a poor design choice. It allows any part of the application to directly access and manipulate the database, bypassing the Database class's interface. This can lead to maintenance issues and hard-to-track bugs. The db handle should remain a private member.

Suggested change

sqlite3* db;

private:

sqlite3* db;

QString file_path;

bool open = false;

private:

sqlite3* db;

QString file_path;

bool open = false;

gemini-code-assist · 2025-09-17T19:14:02Z

fixation.cpp

+    left_pupil_diameter = left_pupil_diameter / double(gaze_count);
+    right_pupil_diameter = right_pupil_diameter / double(gaze_count);


If gaze_vec is empty or contains only invalid gazes, gaze_count will be 0. This will cause a division-by-zero error when calculating left_pupil_diameter and right_pupil_diameter. You should add a check to prevent this.

Suggested change

left_pupil_diameter = left_pupil_diameter / double(gaze_count);

right_pupil_diameter = right_pupil_diameter / double(gaze_count);

if (gaze_count > 0) {

left_pupil_diameter = left_pupil_diameter / double(gaze_count);

right_pupil_diameter = right_pupil_diameter / double(gaze_count);

} else {

left_pupil_diameter = 0;

right_pupil_diameter = 0;

}

gemini-code-assist · 2025-09-17T19:14:03Z

controller.cpp

    for(auto session_id : sessions) {
        //std::cout << "?" << std::endl;
        QVector<Fixation> session_fixations;
+        QVector<Saccade> session_saccades;


The variable session_saccades is declared but its value is never used. This is considered dead code and should be removed to improve code clarity.

gemini-code-assist · 2025-09-17T19:14:03Z

fixationalgorithm.h

+    //issue 58 - a vector to store our saccades
+    //Also, not sure if we'll need a saccade estimate
+    QVector<Saccade> saccades;
+    Saccade computeSaccadeEstimate(QVector<Gaze>);


The function computeSaccadeEstimate is declared as a non-virtual member function in the base class FixationAlgorithm, but it is not implemented here. This will cause a linker error if it's ever called on a FixationAlgorithm pointer/reference that points to a derived class that doesn't implement it (like BasicAlgorithm). To enforce that derived classes provide an implementation, you should declare it as a pure virtual function, similar to computeFixationEstimate.

Suggested change

Saccade computeSaccadeEstimate(QVector<Gaze>);

virtual Saccade computeSaccadeEstimate(QVector<Gaze>)=0;

gemini-code-assist · 2025-09-17T19:14:03Z

ivtalgorithm.cpp

+    std::vector<double> velocity_vector;
+    velocity_vector.push_back(0);
+
+    for(int i = 1; i < session_gazes.size(); ++i) {
+        velocity_vector.push_back(calculateGazeVelocity(session_gazes[i-1].x,session_gazes[i-1].y,session_gazes[i].x,session_gazes[i].y));
+    }


The logic to calculate velocity_vector is duplicated from generateFixations(). To avoid code duplication and improve maintainability, consider calculating this vector once, perhaps in the constructor, and storing it as a member variable.

gemini-code-assist · 2025-09-17T19:14:03Z

ivtalgorithm.cpp

+    for(auto point : saccade_points) {
+
+        saccade.start_x=saccade_points.first().x;
+        saccade.start_y=saccade_points.first().y;
+        saccade.end_x=saccade_points.last().x;
+        saccade.end_y=saccade_points.last().y;
+
+        dx=saccade_points.last().x-saccade_points.first().x;
+        dy=saccade_points.last().y-saccade_points.first().y;
+
+        saccade.amplitude=sqrt(dx*dx+dy*dy);
+        saccade.peak_velocity=calculatePeakVelocity(saccade_points);
+        saccade.avg_velocity=calculateAverageVelocity(saccade_points);
+
+
+        saccade.gaze_vec.push_back(point);
+
+
+    }


The calculations for saccade properties (start_x, amplitude, etc.) are inside a for loop that iterates over every gaze point. This is inefficient as these properties can be calculated once for the entire set of points. Consider moving these calculations outside the loop.

saccade.gaze_vec.reserve(saccade_points.size()); for(auto point : saccade_points) { saccade.gaze_vec.push_back(point); } if (!saccade_points.isEmpty()) { saccade.start_x=saccade_points.first().x; saccade.start_y=saccade_points.first().y; saccade.end_x=saccade_points.last().x; saccade.end_y=saccade_points.last().y; dx=saccade_points.last().x-saccade_points.first().x; dy=saccade_points.last().y-saccade_points.first().y; saccade.amplitude=sqrt(dx*dx+dy*dy); saccade.peak_velocity=calculatePeakVelocity(saccade_points); saccade.avg_velocity=calculateAverageVelocity(saccade_points); }

gemini-code-assist · 2025-09-17T19:14:03Z

ivtalgorithm.h

    int velocity_threshold;
    int duration_ms;
+
+    Database db;


The Database db; member is declared but it is never used within the IVTAlgorithm class. This is dead code and should be removed to improve clarity and reduce the object's memory footprint.

ManOfNeptune27 and others added 3 commits July 22, 2025 18:17

Initial implementation of saccade tracking.

29af7d0

Updated attempt at Saccade issue with added parameters

29aca03

Fixing merge issue with class Saccade implementation

967bfd1

There was a problem with merging that needed to be fixed.

gemini-code-assist bot reviewed Sep 17, 2025

View reviewed changes

Merge branch 'issue-58-saccades' into conners-issue-58-saccade-work

429f26e



		left_pupil_diameter += isnan(gaze.left_pupil_diameter) \|\| gaze.left_pupil_diameter == -1.0 ? 0 : gaze.left_pupil_diameter;
		right_pupil_diameter += isnan(gaze.right_pupil_diameter) \|\| gaze.left_pupil_diameter == -1.0 ? 0 : gaze.right_pupil_diameter;

		left_pupil_diameter = left_pupil_diameter / double(gaze_count);
		right_pupil_diameter = right_pupil_diameter / double(gaze_count);

	Saccade computeSaccadeEstimate(QVector<Gaze>);
	virtual Saccade computeSaccadeEstimate(QVector<Gaze>)=0;

Conners issue 58 saccade work #75

Are you sure you want to change the base?

Conners issue 58 saccade work #75

Uh oh!

Conversation

ManOfNeptune27 commented Sep 17, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants