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

b2Chain_GetSegments is missing #5

Closed
mreinstein opened this issue Jan 6, 2025 · 12 comments
Closed

b2Chain_GetSegments is missing #5

mreinstein opened this issue Jan 6, 2025 · 12 comments

Comments

@mreinstein
Copy link
Contributor

see https://github.com/erincatto/box2d/blob/f377034920c42a26cd498c0a0b1b2e9f2b064989/include/box2d/box2d.h#L676

The commit that added it: erincatto/box2d@df7373c

@mreinstein mreinstein changed the title b2_GetChainSegments is missing b2Chain_GetSegments is missing Jan 6, 2025
@mreinstein
Copy link
Contributor Author

With this missing there doesn't seem to be any way to iterate over a box2d chain and access it's shapes.

@mreinstein
Copy link
Contributor Author

mreinstein commented Jan 6, 2025

@photonstorm I'm happy to submit a PR for this, but I'm curious if there is a more rigorous methodology y'all are using to track commits on the upstream box2d repo. I'm assuming you don't just want me to start ripping missing functions into phaser-box2d.

@photonstorm
Copy link
Contributor

photonstorm commented Jan 6, 2025

@mreinstein as you know, upstream commits are.. messy, to say the least. Take a look at erincatto/box2d@87e13e4 for a perfect example. There are parts of that we need and lots of it we don't. Often test functions are merged in with essential updates. So far we've been tracking the commit, deciding which parts of it are worth converting and then ticking them off one by one (I exposed the most recent changes for this in the CHANGELOG.md).

So basically, yes, please feel free to submit a PR, and let me know exactly which commit it came from so I can then check to see if that completes the entire commit, or just a section of it!

@mreinstein
Copy link
Contributor Author

Yes of course. I wasn't suggesting every commit should be copied verbatim from box2d's c implementation to this project. Obviously there are some things like SIMD and threading, etc. that don't have an analog or aren't appropriate for a js runtime. :-)

I'm speaking more to the state of the current port, and if there's some formal process going forward in terms of tracking how many commits need to be reviewed/ported over. How is this tracked now? Is there a "last commit synced" hash that is being stored in an .md?

I was speaking to the person that was initially contracted to port v3 (PeteB on discord), and he mentioned that it's been in a messy state, due to the fact that the v3 port was still in progress when he started, and there were a pile of commits that may have slipped through the cracks. Has anyone identified which commits might be missing, so that we could get to a state where there's a last known commit hash to work from?

@leeoniya
Copy link

leeoniya commented Jan 6, 2025

i had similar thoughts floating around, since the project does not mention if it's up to date with 3.1 or some in-between state past 3.0.

i imagine that if tests exist to exercise new features/apis it should make it easier to see what is and isnt implemented. this doesnt work for optimizations/refactors, but it's something

@photonstorm
Copy link
Contributor

That's kind of what I was saying. It's not as easy as 'last commit' - as a single commit can have many, many different changes bundled inside of it. Atomic, they are most certainly not. A single commit requires a lot of effort to unpick into 'bug fixes', 'new features' and 'changes on a whim'. So we need to be far more granular than that before we can consider an entire commit done.

The current codebase comes from the v3.0.0 tagged release (there is no 3.1 yet) and the changelog lists the upstream commits since that point. Ticked ones are done, the remainder need evaluating. It may actually be easier to wait for an actual 3.1 release and re-sync at that point, but some elements would be good to include now (bbox perf, capsule collider updates, etc)

@mreinstein
Copy link
Contributor Author

mreinstein commented Jan 6, 2025

It's not as easy as 'last commit'

It is that easy in terms of marking phaser's maintenance of it, right? e.g., box2d is at commit 108, phaser has last reviewed commits all the way up until commit 104. That would mean phaser box2d maintenance folks need to review 4 commits, and mark somewhere "hey someone has reviewed commits up until 108" so that in the future other maintainers know where to start from.

I'm asking if we have done that yet, or if we plan to. Without that we're going to be running around in circles I think.

@photonstorm
Copy link
Contributor

As I said, the commit tagged v3.0.0 is the one that can be considered the base point. Which is 769. That's the point at which everything was converted. But there are several commits beyond that which have also been completed, and several parts of other commits, too, because we merged small parts based on the issue/s it resolved, not the commit as a whole.

There's an argument to be made that we should freeze it at 3.0.0, upstream bugs and all, and not touch it again until the next official version is released. Or we'll forever be in a strange no-mans land, like the upstream master branch is.

@mreinstein
Copy link
Contributor Author

Which is 769.

So it sounds like what someone needs to do is start from 769, and evaluate what (if anything) was missed moving forward to present latest commit on the box2d repo.

@mreinstein
Copy link
Contributor Author

There's an argument to be made that we should freeze it at 3.0.0

I don't think the cadence of it matters too much. What strikes me as most important is:

  • we ensure that we process/merge every commit (even if there's nothing in that commit that's relevant to the js port, knowing it's been evaluated)
  • we record that somewhere
  • we regularly keep up with the commit stream

how those get packaged in phaser-box2d seems somewhat arbitrary. 🤷

@mreinstein
Copy link
Contributor Author

Transitioned this discussion to a separate ticket so it's not continuing to munge this into b2Chain_GetSegments which is technically a separate topic. #8

@mreinstein
Copy link
Contributor Author

mreinstein commented Jan 9, 2025

@photonstorm I don't know if you folks use the assignfeature in this issue tracker but if so, feel free to assign this one to me.

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

No branches or pull requests

3 participants