Haven't checked in detail yet, but started the buildbot here: https://builder.blender.org/admin/#/builders/18/builds/194

Buildbot is happy, is this good to merge?

Doing the first change to geometry nodes we decided to use the file name as the namespace. Looks close here but it's using an abbreviation for "composite". I don't have a strong opinion, but I think it would be simpler and easier to automate such things if thr namespace and file names match?

Also, in the patch title, "there" -> "their" ;)

I don't have a strong opinion, but I think it would be simpler and easier to automate such things if thr namespace and file names match?

Would it be wise to also use the abbreviation in the file names? Current file names are a bit long IMO but not torn because at the same time I don't like abbreviations too much.

Also, some file names are camel case or have all caps sepcombHSVA.cc vecBlur.cc I kept namespace names all lower case. I suppose the file names should be made all lower case aswell.

Also, should some nodes be split up such as the sepcomb nodes?

I think we should follow the convention of using file names as namespace names. The reason for the file namespaces is to support unity builds, otherwise they would not be necessary (supporting unity build for compositor nodes would be great).
For files that contain more than one node, you could put the individual nodes into yet another namespace or split the nodes into separate files.
I think the files should be split up, but that doesn't have to be part of this patch. Also the file names should be lower case of course.

Generally, I think you should talk to someone else working on the compositor before merging this. While Hans and I are doing lots of work on the node systems, the compositor code is not really maintained by us.

I think Aaron changed the shader node patch to use the file names, I think that was planned here too?

In D13466#356283, @Hans Goudey (HooglyBoogly) wrote:

I think Aaron changed the shader node patch to use the file names, I think that was planned here too?

Yes in fact, I made the changes before that comment I just forgot to update the patch.

Okay, the namespaces still don't match the file names though, using cmp instead of composite.

My bad should be good now

Is @Manuel Castilla (manzanilla) still a blocking reviewer here?

Well, I agree with Jacques that someone more deeply involved with the compositor should at least be clued into the work that's going on here.

Including Jeroen as a reviewer, as Manuel has not been as active recently.

Updating patch to master soon...

I find it strange that namespaces have a postfix _cc but seems the same for geo nodes and is described in the previous patch.

Personally I would use lower case only in namespaces + filenames (except prefix). I think it is better to use proper namespaces and rename the files to use lower case names where needed. as in node_composite_alpha_over.

Did some small tests with cryptomatte and it seems to work as expected.

What I miss is a bit of the planning, but that is more related to the module. We do this for possible future tooling. But there is no mention if these tools are actually planned...

What needs to be done for me is renaming the files so we don't have to do another cleanup that could break this convention as there isn't any tooling.

In D13466#362993, @Jeroen Bakker (jbakker) wrote:

I find it strange that namespaces have a postfix _cc but seems the same for geo nodes and is described in the previous patch.

I have no preference here, but I would like this to be consistent between the rest of the node declarations.
So I would like to here from @Jacques Lucke (JacquesLucke) here, also see the rational given in rB1df8abff257030ba79bc23dc321f35494f4d91c5

Personally I would use lower case only in namespaces + filenames (except prefix). I think it is better to use proper namespaces and rename the files to use lower case names where needed. as in node_composite_alpha_over.

Sounds good, as soon as this patch is merged I was planning on addressing the file names regardless.

What I miss is a bit of the planning, but that is more related to the module. We do this for possible future tooling. But there is no mention if these tools are actually planned...

What do you mean by future tooling? Do you mean enabling unity builds? or something else. Do you mean a tool to automatically add namespace based on the filename?

What needs to be done for me is renaming the files so we don't have to do another cleanup that could break this convention as there isn't any tooling.

Seems reasonable, I was planning on renaming these files anyways because I have an understanding of the convention there wouldn't be any breakage, however, I see that this could be an issue in the future.

So based on the above, the plan is to use correct namespace names in the patch then once merged follow up and rename files to match the new namespace.

Do I understand this correctly @Jeroen Bakker (jbakker) ?

The tooling I mentioned is from the related change. {1df8abff2570}

That makes it possible to write some tooling that checks the names later on.

Having some direct link between filesnames and name spaces is ok for traceability. But don't having a tool or a coding pattern in place that enforce the restriction would lead to that the rule isn't applied to new code/changes as it isn't clear to the developer.
For example when a new compositor/shader node is added it normally doesn't go through review of a nodes developer, who are aware of the mentioned rules.

Personally I would remove the _cc postfix. as the tooling isn't in place or designed yet, but somehow we are already taken into account that the tooling that we don't have requires it. I rather don't confuse developers.

But that should be something that @Jacques Lucke (JacquesLucke) might be able to clarify.

I started writing a small script that can check the namespace names and corrects them if necessary (D13752). It also allows inserting a comment before every namespace declaration, but that can be used later.

The script found issues in three files:

• node_composite_brightness.cc
• node_composite_curves.cc
• node_composite_lumma_matte.cc