search

pema99 / UnityShaderParser

BSD 3-Clause "New" or "Revised" License
17 stars 2 forks source link

HLSLSyntaxVisitor.VisitMany<T> can never return anything but default #4

Closed HB-Stratos closed 2 months ago

HB-Stratos commented 2 months ago

I've spent a while now trying to learn how to use this library, and how to implement Visit[nodename]() override functions. One of the neat functionalities that seems to have been intended originally was the option of implementing such an override with a return type that would presumably be handed down the chain such that customSyntaxVisitor.VistMany() would return a list of values.

However, this currently does not work, at least not recursively. If I have a struct that contains a float, and I implement an override 

public override string VisitScalarTypeNode(ScalarTypeNode node)
{
    return PrintingUtil.GetEnumName(node.Kind);
}

I will never get this value out of VisitMany(), instead I always get null. I believe this is due to the implementation here https://github.com/pema99/UnityShaderParser/blob/4e2056b883c5cf1b0e0587957503120d1604456a/UnityShaderParser/HLSL/HLSLSyntaxVisitor.cs#L113-L122 never returning any value. child.Accept() is called, but it's return value is discarded and instead default is returned.

I have attempted to remedy this in my implementation, but due to DefaultVisit not being virtual or static, it can not be overwritten, so the only path to solution would be to copy-paste the entire chunk of visit functions and change each of their functions from DefaultVisit() to a custom one.

pema99 commented 2 months ago

Check my answer to the other issue opened for something that works - there are 2 versions of HLSLSyntaxVisitor. A generic and non-generic one.

But yes, you have spotted an oversight. DefaultVisit was intended to be virtual. I'll fix it now.

HB-Stratos commented 2 months ago

Your other answer arrived as I was writing this. Very helpful, I'll end up implementing that. Still nice that this is getting fixed though. As far as I can tell no tests use the generic visitor, and the printer also just uses the void visitor, so I couldn't find any references on how to use it. After a couple hours of tracing things, this is what I found.

pema99 commented 2 months ago

Fixed here

https://github.com/pema99/UnityShaderParser/commit/ff3ec32ab01494ac34eccfe01111a2347c5b9f26

https://github.com/pema99/UnityShaderParser/releases/tag/0.3.3

HB-Stratos commented 2 months ago

Awesome, thank you!

pema99 commented 2 months ago

As far as I can tell no tests use the generic visitor, and the printer also just uses the void visitor

Yes. It isn't a goal of mine to have full coverage, just what is useful for my own development. This library was something I wrote for my own use, but decided to put on GitHub since I figured some of it may be useful to other. As such, it doesn't live up to the same standards as production grade parsing libraries, and I can only provide limited support in my spare time. I'm sure you get my gist.