-
Notifications
You must be signed in to change notification settings - Fork 193
Conversation
Sorry for assigning so many reviewers - not sure who was best to review since the PR is a bit broad (github was suggesting you guys). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave a review to the backends except msl
(since I know next to nothing about it), there are problems that need to be fixed.
Also this shouldn't have been a monolith PR but a couple of PRs , it makes the life of everyone worse, you don't get as good nor a timely review, fixing the problems in the right places requires rebasing and other shenanigans, the reviewers need to look at a lot of unrelated code, it's more probable of causing/having merging problems with other PRs and CI will only run on the last commit which will either require the reviewers to test every commit or it to be squashed which is bad because we have lot of changes and losing history on it would be bad.
So in the future if the changes aren't related please open separate PRs, you should also do this for commits, for example you have add support for unary vector operators
which could have a been a commit for each backend, which would reduce the diff to only the code and tests changes for that specific backend.
I only mean this as constructive criticism
While I generally try to keep PRs as focused as possible, this one is in somewhat of a grey area form me. I started by adding a bunch of tests and then with each commit slowly working out the problems. I understand the concerns and can split it up but at least in my opinion it didn't reach a threshold where this is necessary (I find it easier to see what changed and what the fixes/improvements are with PRs of this kind; i.e. where tests are added and fixes are implemented incrementally). |
@JCapucho tbh, your first comment left a sour taste in my mouth. It comes off as aggressive even after rereading it a few times and I'm not sure why you felt the need to phrase it like that since this is our first interaction ("I only mean this as constructive criticism" is just a cop-out; if it weren't it wouldn't have been necessary in the first place). Regarding the content; if you feel so strongly about splitting commits and PRs as much as possible I would recommend adding a |
I'm very sorry if it does come out as aggressive, English isn't my native language so sometimes I do have trouble expressing myself, I have no ill intent towards you I only wish to make the best shader translation library in the world :)
I think this is a problem of different cultures, in my mother tongue it's very common to say that after a longer comment on someone's work (and it comes out as aggressive if you don't say it).
I do feel strongly about it and I have already expressed why, I think it makes everyone's life easier. Once again I'm very sorry @teoxoy if I sounded aggressive, I truly mean it when I say it wasn't my intention and thanks for pointing it out, I'll try to improve how I communicate. |
I appreciate the reply. Communication is hard 😅 - especially between different cultures. All good!
Big props! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teoxoy Looked at glsl, hlsl, and msl changes. SPIR-V scares me, and I don't know much about it :) More tests are great! Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted above I didn't review msl
, but the other parts seem good to me.
Matrix + Matrix
operations not implemented #1527)~
,|
and&
ops