Skip to content

Multiple textures#992

Closed
tzukpolinsky wants to merge 4 commits intostevenlovegrove:masterfrom
tzukpolinsky:multiple-textures
Closed

Multiple textures#992
tzukpolinsky wants to merge 4 commits intostevenlovegrove:masterfrom
tzukpolinsky:multiple-textures

Conversation

@tzukpolinsky
Copy link
Contributor

Hi
this is the code that enables multiple textures
As you can see, it mainly takes the current logic and put it into a for loop.

i hope that you would accept the shader.h as well. when i started working with shaders and didnt know anything about it, this examples really helped me and i hope they would help others in the future :)

@christian-rauch
Copy link
Collaborator

Thanks. I think this PR may need rebasing or similar. It shows conflicts and an empty commit. I think this may be because you are trying to merge a commit that is already upstreamed.

@christian-rauch christian-rauch mentioned this pull request Aug 24, 2025
@tzukpolinsky
Copy link
Contributor Author

Sorry this isnt my strong side, so i should take this branch, rebase it to the current pangolin version and then merge my changes to it and after the do a PR?

@christian-rauch
Copy link
Collaborator

You have to fetch all changes from the upstream branch, i.e. this repo, (git fetch --all -p), check out this branch from your fork (git checkout multiple-textures), then rebase on top of the upstream master (git rebase origin/master), assuming origin is https://github.com/stevenlovegrove/Pangolin. This will likely show that you have conflicts that need to be resolved.

I am not able to push to your PR branch, so you will have to do these changes. If you cannot manage, I will try to do this in a separate PR of my own.

@tzukpolinsky
Copy link
Contributor Author

i think i have done it right, but let me know if something is wrong.
i did the following steps:
git remote add upstream https://github.com/stevenlovegrove/Pangolin
git fetch --all -p
git checkout multiple-textures
git rebase upstream/master

and took my changes in the relevant files.

@christian-rauch
Copy link
Collaborator

Not sure what happened. Some of your commits appear twice in the PR now. Maybe just do an interactive rebase (git rebase -i upstream/master) and only keep the first commit. Then force push (git push -f).

@tzukpolinsky
Copy link
Contributor Author

i hope now we good:)

Copy link
Collaborator

@christian-rauch christian-rauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

There is only a minor nit-pick: The rest of the code uses left-aligned pointer and reference operators, i.e. type& var:, instead of right-aligned, i.e. type &var:.

This is not a big issue, but if you can, can you move them to the left side? If not, I will just marge as is.

@tzukpolinsky
Copy link
Contributor Author

sorry i cant find it in the codebase. i looked into glgeometry.cpp and geometry_obj.cpp.

Copy link
Collaborator

@christian-rauch christian-rauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented on some examples of what I mean of shifting the reference operator from right to left.

@tzukpolinsky
Copy link
Contributor Author

Done

Copy link
Collaborator

@christian-rauch christian-rauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you squash your commits together, i.e. squash the new fixup commit into your original commit so that they appear as one? Also, see my comments about the "newline" where the EOL was remove in your last commit.

}

}
} No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the EOL here back again?

}

}
} No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the EOL here back again?

@tzukpolinsky
Copy link
Contributor Author

I hope this is what you are looking for, I feel like we are investing a lot of time into this...
Hope this would get your approval.

@christian-rauch
Copy link
Collaborator

It would still be nice to have this squashed and rebased again, as with the commits before. But I can take care of this, if you do not have the time.

@tzukpolinsky
Copy link
Contributor Author

Thank you, I admit I feel that i following the instructions of AI and you without fully understanding the bigger picture of what I do:)

I think it would be better for the repo if you would continue this, because i can build the project on my computer and run tests and i dont want to inject errors into your code:)

Sorry, it's been so long since I worked on this project.
Thank you for you effort as well, this package really helped my research and we build a really nice drone simulator with it:)
https://github.com/rbdlabhaifa/simulatorMapping/commits/main/

Thank you!

@christian-rauch
Copy link
Collaborator

Replaced by #995.

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