Skip to content

Feat: Add wrap and neighbor access methods for grid logic#36

Open
HossamSaberr wants to merge 4 commits intopharo-containers:masterfrom
HossamSaberr:feature-wrap-neighbors
Open

Feat: Add wrap and neighbor access methods for grid logic#36
HossamSaberr wants to merge 4 commits intopharo-containers:masterfrom
HossamSaberr:feature-wrap-neighbors

Conversation

@HossamSaberr
Copy link
Copy Markdown
Contributor

This PR implements the boundary and adjacency logic.

  • Wrapping: Added atColumnWrap:atRowWrap: and atColumnWrap:atRowWrap:put: to allow seamless boundary wrapping using 1-based modulo arithmetic.

  • Neighbors: Added neighborsAtColumn:atRow: to safely fetch up to 8 surrounding valid elements without out-of-bounds errors.

  • Testing: Added tests in CTArray2DTest covering normal access, edge wrapping and a full 8-neighbor.

  • Fixes About wrap and neighbors #17

@jordanmontt
Copy link
Copy Markdown
Member

This PR has conflicts now :/

@HossamSaberr
Copy link
Copy Markdown
Contributor Author

Resolved!

@jordanmontt
Copy link
Copy Markdown
Member

@Ducasse could you review this, please?

@virenvarma007
Copy link
Copy Markdown

@HossamSaberr Nice work on this PR overall.
The wrapping API (atColumnWrap:atRowWrap: and ...put:) is clean, and the added tests for wrapping reads/writes plus neighbor behavior are solid.

@Ducasse one small follow-up point: issue #17 mentions neighbors “with or without wrapping at edges.” Right now we have non-wrapping neighbors and wrapping for direct access, but not a wrapped-neighbors variant yet. Also, neighborsAtColumn:atRow: does not currently enforce in-bounds origin coordinates, so clarifying expected behavior there would help. What do you think?

@HossamSaberr
Copy link
Copy Markdown
Contributor Author

Thanks for the review, @virenvarma007!

Spot on with the wrapped variant, I'll add neighborsAtColumnWrap:atRowWrap: to handle that case

the out-of-bounds origin for neighborsAtColumn:atRow:: I think throwing an error here is much better

I'll push the updates for both of these, along with the tests

@HossamSaberr
Copy link
Copy Markdown
Contributor Author

I've pushed the updates to address the points you raised @virenvarma007
The CI and Tests all Green, ready for another look

@virenvarma007
Copy link
Copy Markdown

This looks really good to me. Although we should wait for @Ducasse ’s take before we call it fully done, but either way, @HossamSaberr thank you for the effort you put into this.

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.

About wrap and neighbors

3 participants