Add support for OpenPGL 0.5.0 #106861

Sebastian Parborg · 2023-04-12T16:06:48+02:00

Sebastian Parborg commented

2023-04-12 16:06:48 +02:00

Some functions changed slightly for this non beta release.
No functional changes though as we didn't use what was removed.

Some functions changed slightly for this non beta release. No functional changes though as we didn't use what was removed.

Sebastian Parborg added the

Interest

Cycles

label 2023-04-12 16:06:48 +02:00

Sebastian Parborg added 1 commit 2023-04-12 16:07:01 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

9ed6647edb Add support for OpenPGL 0.5.0

Some functions changed slightly for this non beta release.
No functional changes though as we didn't use what was removed.

Sebastian Parborg requested review from Brecht Van Lommel 2023-04-12 16:07:43 +02:00

Sebastian Parborg requested review from Sergey Sharybin 2023-04-12 16:07:43 +02:00

Sebastian Parborg commented

2023-04-12 16:09:18 +02:00

BTW, should we perhaps cherry pick this for the 3.5.1 release?
OpenPGL 0.5.0 seems to be the first non beta release of the software, so I'm guessing that is what most distros will package eventually.

BTW, should we perhaps cherry pick this for the 3.5.1 release? OpenPGL 0.5.0 seems to be the first non beta release of the software, so I'm guessing that is what most distros will package eventually.

Sebastian Parborg commented

2023-04-12 16:09:56 +02:00

@blender-bot build

Sebastian Parborg added

and removed

labels 2023-04-12 16:30:57 +02:00

Sergey Sharybin requested changes 2023-04-12 17:34:56 +02:00

Sergey Sharybin left a comment

The version kind of works for now, but on a logic level it is wrong: if the major version 1.0 is released tomorrow (hypothetically) without changing this part of the API the wrong code path will be taken. Strictly speaking this needs to be OPENPGL_VERSION_MAJOR >= 1 || OPENPGL_VERSION_MINOR >= 5.

For the cherry-pick the biggest concern I have is that it is unclear how well tested the change is, and how well Cycles works with the new OpenPGL release.

The version kind of works for now, but on a logic level it is wrong: if the major version 1.0 is released tomorrow (hypothetically) without changing this part of the API the wrong code path will be taken. Strictly speaking this needs to be `OPENPGL_VERSION_MAJOR >= 1 || OPENPGL_VERSION_MINOR >= 5`. For the cherry-pick the biggest concern I have is that it is unclear how well tested the change is, and how well Cycles works with the new OpenPGL release.

Sebastian Parborg commented

2023-04-12 18:00:56 +02:00

The version kind of works for now, but on a logic level it is wrong: if the major version 1.0 is released tomorrow (hypothetically) without changing this part of the API the wrong code path will be taken. Strictly speaking this needs to be OPENPGL_VERSION_MAJOR >= 1 || OPENPGL_VERSION_MINOR >= 5.

I actually had that initially, but then I looked at what the current checks in the code does and they only check for the minor version. (Which I agree is wrong, but I didn't want to go against the implicit standard that is currently in the code)

Perhaps I should create a follow up commit in this PR that fixes all the remaining checks as well?

For the cherry-pick the biggest concern I have is that it is unclear how well tested the change is, and how well Cycles works with the new OpenPGL release.

Are there any built in test for this?
All cycles tests seems to pass on my computer with 0.5.0. But I don't know if you are testing PGL specifically in any of them right now.

> The version kind of works for now, but on a logic level it is wrong: if the major version 1.0 is released tomorrow (hypothetically) without changing this part of the API the wrong code path will be taken. Strictly speaking this needs to be `OPENPGL_VERSION_MAJOR >= 1 || OPENPGL_VERSION_MINOR >= 5`. I actually had that initially, but then I looked at what the current checks in the code does and they only check for the minor version. (Which I agree is wrong, but I didn't want to go against the implicit standard that is currently in the code) Perhaps I should create a follow up commit in this PR that fixes all the remaining checks as well? > For the cherry-pick the biggest concern I have is that it is unclear how well tested the change is, and how well Cycles works with the new OpenPGL release. Are there any built in test for this? All cycles tests seems to pass on my computer with 0.5.0. But I don't know if you are testing PGL specifically in any of them right now.

Sergey Sharybin commented

2023-04-12 18:19:16 +02:00

I actually had that initially, but then I looked at what the current checks in the code does and they only check for the minor version. (Which I agree is wrong, but I didn't want to go against the implicit standard that is currently in the code)

Perhaps I should create a follow up commit in this PR that fixes all the remaining checks as well?

I see. For now it is indeed more consistent to follow the MINOR check only.

For the more clear checks we probably should introduce something like OPENPGL_VERSION_AT_LEAST(...) in the util/guiding.h. It will make checks in the code be much easier to understand.

Are there any built in test for this?

There are some tests, but I am not sure if things like #104329 have a dedicated test.
There is also difference between a handful automated tests, and actual real world usage. The latter one we simply do not have any feedback yet.

That being said, fixing compilation error is fine for the corrective release, granted it is straightforward change. Is more of setting an expectation: while Blender will be able to be compiled, we can not guarantee it works same reliable as the official Blender release.

> I actually had that initially, but then I looked at what the current checks in the code does and they only check for the minor version. (Which I agree is wrong, but I didn't want to go against the implicit standard that is currently in the code) > > Perhaps I should create a follow up commit in this PR that fixes all the remaining checks as well? I see. For now it is indeed more consistent to follow the MINOR check only. For the more clear checks we probably should introduce something like `OPENPGL_VERSION_AT_LEAST(...)` in the `util/guiding.h`. It will make checks in the code be much easier to understand. > Are there any built in test for this? There are some tests, but I am not sure if things like #104329 have a dedicated test. There is also difference between a handful automated tests, and actual real world usage. The latter one we simply do not have any feedback yet. That being said, fixing compilation error is fine for the corrective release, granted it is straightforward change. Is more of setting an expectation: while Blender will be able to be compiled, we can not guarantee it works same reliable as the official Blender release.

Sergey Sharybin approved these changes 2023-04-12 18:20:14 +02:00

Sergey Sharybin left a comment

And because there is no easy way to write a reply in the conversation and approve the code at the same time., here is a dedicated comment from the code review section.

Brecht Van Lommel requested review from Ghost Team 2023-04-12 20:55:17 +02:00

Ghost commented

2023-04-12 21:20:49 +02:00

First-time contributor

@ZedDB, your patch looks exactly like the one I wanted to push tomorrow :)

@Sergey as soon as the API is more stable and all Blender dependency builds are upgraded to >= 0.5 I would like to remove some of the version ifdef and bump the minimum required Open PGL version.

@ZedDB, your patch looks exactly like the one I wanted to push tomorrow :) @Sergey as soon as the API is more stable and all Blender dependency builds are upgraded to >= 0.5 I would like to remove some of the version ifdef and bump the minimum required Open PGL version.

Sergey Sharybin commented

2023-04-13 09:49:52 +02:00

@Sergey as soon as the API is more stable and all Blender dependency builds are upgraded to >= 0.5 I would like to remove some of the version ifdef and bump the minimum required Open PGL version.

Sure, once we bump requirement we can get rid of the current checks.
But I can imagine that in the future we'll want to be able to check version in the future, with the new development of OpenPGL. For that I'd really prefer having more explicit comparison than relying on an implicit match of the major version.

> @Sergey as soon as the API is more stable and all Blender dependency builds are upgraded to >= 0.5 I would like to remove some of the version ifdef and bump the minimum required Open PGL version. Sure, once we bump requirement we can get rid of the current checks. But I can imagine that in the future we'll want to be able to check version in the future, with the new development of OpenPGL. For that I'd really prefer having more explicit comparison than relying on an implicit match of the major version.

Sebastian Herholz approved these changes 2023-04-13 10:19:40 +02:00

Sebastian Herholz left a comment

This looks good.

Sebastian Parborg referenced this issue from a commit

2023-04-13 11:44:47 +02:00

Add support for OpenPGL 0.5.0

Sebastian Parborg merged commit aa6e95281f into main

2023-04-13 11:44:47 +02:00

Sebastian Parborg deleted branch openpgl5

2023-04-13 11:44:48 +02:00

Sebastian Parborg referenced this issue from a commit

2023-04-13 11:59:18 +02:00