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

Refactor tracker to be class based + Add types #48

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JooZef315
Copy link

@JooZef315 JooZef315 commented Oct 14, 2024

@JooZef315
Copy link
Author

@vsaw hope you review it and merge it to the new release if everything is ok,
thank you

@snowfluke
Copy link

@JooZef315 could you make a fork of it instead? I doubt it will be merged here

@JooZef315
Copy link
Author

@JooZef315 could you make a fork of it instead? I doubt it will be merged here
@snowfluke

It is already forked. If you want to use the fork you may use it with
npm I https://github.com/JooZef315/node-moving-things-tracker
OR
npm i git+ssh://[email protected]:/JooZef315/node-moving-things-tracker.git

@vsaw
Copy link
Collaborator

vsaw commented Dec 8, 2024

Sorry, I'm maintaining ODC and this Library as a hobby and got caught up in my day job. That being said I would like to merge this PR but don't have the time to fully review it therefore let me ask you a few questions and hopefully we can push through this way

  1. is it backwards API compatible or does this require changes on ODC side to work
  2. Could you check if your Code uses the ODC Code Style https://opendata.cam/docs/development/#code-style
  3. Are there any Breaking changes in the libraries you updated as you skipped quite a few versions in your update

@JooZef315
Copy link
Author

1- the breaking change now is that to use the tracker we have to make an instance of it first

import { Tracker } from 'node-moving-things-tracker';

const tracker = new Tracker();
tracker.updateTrackedItemsWithNewFrame(detectionScaledOfThisFrame, currentFrame);
const trackerDataForThisFrame = tracker.getJSONOfTrackedItems();

2- YES, it uses the ODC Code Style

I used the linter to check and it passes well.

3- No Breaking changes in the updated libraries

only uuid now has to imported as const { v4: uuidv4 } = require("uuid");

@vsaw
Copy link
Collaborator

vsaw commented Dec 10, 2024

@JooZef315 Is there a way to keep it backwards compatible? E.g. by having a static default instance that will be used if people don't create separate instances?

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