Skip to content

Conversation

@Thibault-Pelletier
Copy link
Contributor

  • Add event throttle and compression to handle unresponsive server during interaction.

@Thibault-Pelletier
Copy link
Contributor Author

Thibault-Pelletier commented Sep 17, 2025

Hi @jourdain,

This PR adds a event throttling and compression mechanism to avoid overloading the server when it's hanging.

My use case is the following :
When placing markup points in trame-slicer on models with a large number of points (mouse hover + cell picking), the processing lags behind and users need to stop their interaction completely to see any change.

With the event throttling / compression, only the latest mouse hover is sent and others are ignored.

As I'm not a JS developper, this is a port of the equivalent Python impl I tested before but which wasn't satisfactory performance wise. Don't hesitate to let me know what needs to be changed for it to be more JS.

Let me know what you think of this mechanism and how I can improve this PR !

@Thibault-Pelletier Thibault-Pelletier marked this pull request as draft September 17, 2025 14:29
@jourdain
Copy link
Collaborator

In general the code looks great, especially the logic and idea. The only thing that could be optimized would be memory handling to prevent/reduce gc.

Those are optimization we may not need, but would not hurt.

@jourdain
Copy link
Collaborator

Possible code change to test in case it is better...

export class EventThrottle {
  constructor(processCallback, throttleTimeMs = 100) {
    this.eventQueue = [];
    this.processing = false;
    this.throttleTimeMs = throttleTimeMs;
    this.processCallback = processCallback;
    this.eventKeysToIgnore = new Set(['x', 'y']);
  }

  compressEvents(events) {
    if (events.length < 2) return [...events];

    const compressed = [];
    for (let i = 0; i < events.length; i++) {
      const current = events[i];
      const next = events[i + 1];

      if (next && this.canCompressEvents(current, next)) {
        continue;
      }
      compressed.push(current);
    }
    return compressed;
  }

  canCompressEvents(prev, next) {
    if (prev.type === 'MouseWheel' || next.type === 'MouseWheel') return false;
    if (Object.keys(prev).length !== Object.keys(next).length) return false;
    
    for (const key in prev) {
      if (this.eventKeysToIgnore.has(key)) continue;
      if (prev[key] !== next[key]) return false;
    }
    return true;
  }

  sendEvent(event) {
    this.eventQueue.push(event);
    if (!this.processing) {
      this.processing = true;
      this._processEventQueue().catch((err) => {
        console.error('Error in _processEventQueue:', err);
        this.processing = false;
      });
    }
  }

  async _processEventQueue() {
    const compressedEvents = this.compressEvents(this.eventQueue);
    this.eventQueue.length = 0

    const t0 = Date.now()

    for (const event of compressedEvents) {
      await this.processCallback(event);
      await new Promise((resolve) => setTimeout(resolve, 0)); // Yield to event loop
    }

    const dt = this.throttleTimeMs - Date.now() + t0;
    if (dt > 0) {
          await new Promise((resolve) => setTimeout(resolve, dt));
    }

    if (this.eventQueue.length > 0) {
      await this._processEventQueue();
    } else {
      this.processing = false;
    }
  }
}

@Thibault-Pelletier
Copy link
Contributor Author

Thanks for the review and the comments!
You can ignore the recent push, I will update my code with your remarks

@jourdain
Copy link
Collaborator

Another pass (sorry)

function yield() {
  return new Promise((resolve) => setTimeout(resolve, 0));
}

async function sleep(timeMS) {
  if (timeMS < 0) { 
     return;
  }
  return new Promise((resolve) => setTimeout(resolve, timeMS));
}

export class EventThrottle {
  constructor(processCallback, throttleTimeMs = 100) {
    this.eventQueue = [];
    this.processing = false;
    this.throttleTimeMs = throttleTimeMs;
    this.processCallback = processCallback;
    this.eventKeysToIgnore = new Set(['x', 'y']);
  }

  compressEvents(events) {
    if (events.length < 2) return [...events];

    const compressed = [];
    for (let i = 1; i < events.length; i++) {
      const current = events[i - 1];
      const next = events[i];

      if (this.canCompressEvents(current, next)) {
        continue;
      }
      compressed.push(current);
    }
    return compressed;
  }

  canCompressEvents(prev, next) {
    if (prev.type === 'MouseWheel' || next.type === 'MouseWheel') return false;
    if (Object.keys(prev).length !== Object.keys(next).length) return false;
    
    for (let key in prev) {
      if (this.eventKeysToIgnore.has(key)) continue;
      if (prev[key] !== next[key]) return false;
    }
    return true;
  }

  sendEvent(event) {
    this.eventQueue.push(event);
    if (!this.processing) {
      this.processing = true;
      this._processEventQueue().catch((err) => {
        console.error('Error in _processEventQueue:', err);
        this.processing = false;
      });
    }
  }

  async _processEventQueue() {
    const compressedEvents = this.compressEvents(this.eventQueue);
    this.eventQueue.length = 0

    const t0 = Date.now()

    for (const event of compressedEvents) {
      await this.processCallback(event);
      await yield();
    }

    await sleep(this.throttleTimeMs - Date.now() + t0);

    if (this.eventQueue.length > 0) {
      await this._processEventQueue();
    } else {
      this.processing = false;
    }
  }
}

@jourdain
Copy link
Collaborator

Just try my code to make sure I don't introduce issue.

@jourdain
Copy link
Collaborator

with the adaptive wait time, your default eventThrottleMs could be ok.

@Thibault-Pelletier
Copy link
Contributor Author

I'll have a look tomorrow. My implementation is broken somehow at the moment 😅

@Thibault-Pelletier
Copy link
Contributor Author

I just updated the code and everything seems to be working properly!

Just a note, I had to :

  • Change the name for the yield function which is reserved in JS
  • Rollback the change to the compressEvents impl which wasn't working (caught by the unit tests)

I added some unitest and updated the workflow to run them. Let me know if I should move them (wasn't sure which folder structure for tests you use / like best for trame components).

- Add event throttle and compression to handle unresponsive server
  during interaction.
@jourdain
Copy link
Collaborator

I guess the compressEvents is just the i handling that you changed? If so that is fine. I could not spot any other changes compare to the impl I provided...

@jourdain
Copy link
Collaborator

Feel free to merge if you are good to go

@Thibault-Pelletier
Copy link
Contributor Author

I guess the compressEvents is just the i handling that you changed? If so that is fine. I could not spot any other changes compare to the impl I provided...

Yes that's correct, only the i handling was changed to keep the last event when no compression was done.

Feel free to merge if you are good to go

PR is good to go on my end, but I don't seem to have the right to merge it. 😅

@jourdain jourdain merged commit 7c7268b into Kitware:master Sep 18, 2025
7 checks passed
@Thibault-Pelletier
Copy link
Contributor Author

Thx for the merge!

@Thibault-Pelletier Thibault-Pelletier deleted the event_throttle branch September 23, 2025 07:28
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