Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Q: Create internal names for properties? #205

Open
JCash opened this issue May 22, 2022 · 8 comments
Open

Q: Create internal names for properties? #205

JCash opened this issue May 22, 2022 · 8 comments

Comments

@JCash
Copy link
Contributor

JCash commented May 22, 2022

I wonder if creating "internal" variable names is preferable, in order to avoid name clashes.
When I started porting to this api, I immediately got a clash where I had to change my prop name to "EngineProps", since there already was a struct named Engine. The drawback is that I'd rather print "Engine" when presenting the properties.

    rmt_PropertyDefine_Group(Engine, "Engine properties");
    rmt_PropertyDefine_U32(FrameCount, 0, FrameReset, "# frames", &Engine);

An alternative would be just passing the name (not a pointer to a variable), allowing the macro to create
internal names for the variable:

    rmt_PropertyDefine_U32(FrameCount, 0, FrameReset, "# frames", Engine);

E.g. the variables could be prefixed and end up like _rmt_prop_Engine and _rmt_prop_FrameCount.

What are your thoughts on this?

@JCash
Copy link
Contributor Author

JCash commented May 22, 2022

Ofc, this complicates things a bit given that the group name is not explicitly used in the macros, but simply passed along with the variable arguments.

@dwilliamson
Copy link
Collaborator

A naming convention could fix this. So you have EngineProperties instead of Engine and in registration various patterns are searched for, like "ending in Properties", where it cuts the name to leave Engine.

Last thing I want to do, though, is add alternate macros for parented vs. non-parented. Maybe another one is every property has to be parented and we just introduc a dummy Root property group. That way you can do property prefixing and not need a naming convention.

@JCash
Copy link
Contributor Author

JCash commented May 24, 2022

Yeah, I guess that's what I'll end up doing (the naming convention).
Feels like the least hassle.

@JCash JCash closed this as completed May 24, 2022
@dwilliamson
Copy link
Collaborator

There is always this option:

#include <stdio.h>

#define EXPAND(x) x
#define VA_NARGS_IMPL(_1, _2, _3, NARGS, ...) NARGS
#define VA_NARGS(...) EXPAND(VA_NARGS_IMPL(__VA_ARGS__, 3, 2, 1, 0))

#define JOIN2(x, y) x ## y
#define JOIN(x, y) JOIN2(x, y)

#define PROP2(prop, parent) puts(#prop " " #parent)
#define PROP1(prop) puts(#prop)
#define PROP(...) JOIN(PROP, VA_NARGS(__VA_ARGS__))(__VA_ARGS__)

int main()
{
    printf("%d\n", VA_NARGS(1, 2, 3));

    PROP(PropA, Parent);
    PROP(PropB);
}

Can be tested here: https://godbolt.org/z/bTxPc39n1

I explain it in a really old blog post: https://raw.githubusercontent.com/dwilliamson/b/623a4882561c0893e01488730896c6660794e257/Posts/2011-07-15-10-23-25.txt

@dwilliamson dwilliamson reopened this May 24, 2022
@JCash
Copy link
Contributor Author

JCash commented May 24, 2022

All properties will still have that prefix when presented in the Html page? (unless I'm mistaken?)

I think the easiest for me is to somehow add my own versions of the defines. E.g. a version of _rmt_PropertyDefine:

#define rmt_propName(name) JOIN(_rmt_p, name)

#define _rmt_PropertyDefine(type, name, default_value, flags, desc, ...) \
    rmtProperty rmt_propName(name) = { RMT_FALSE, RMT_PropertyType_##type, default_value, flags, #name, desc, default_value, __VA_ARGS__ };

And then set the parent like so:

rmt_PropertyDefine_U32(FrameCount, 0, FrameReset, "# frames", &rmt_propName(Engine));

@dwilliamson
Copy link
Collaborator

You could remove the _rmt_prop_ prefix before it gets used for the viewer, here:

Remotery/lib/Remotery.c

Lines 9015 to 9018 in 5d6fb51

// Calculate the name hash and send it to the viewer
name_len = strnlen_s(property->name, 256);
property->nameHash = MurmurHash3_x86_32(property->name, name_len, 0);
QueueAddToStringTable(g_Remotery->mq_to_rmt_thread, property->nameHash, property->name, name_len, NULL);

@JCash
Copy link
Contributor Author

JCash commented May 24, 2022

That is true, and I think if I need to change the source anyways, modifying the "QueueAddToStringTable" part might be the easiest.

@JCash
Copy link
Contributor Author

JCash commented May 30, 2022

For those interested, here's the patch that I apply each time I now update the Remotery.c.
(I prefix our properties rmtp_):

diff --git a/engine/dlib/src/remotery/Remotery.c b/engine/dlib/src/remotery/Remotery.c
index 7514d8d84..b6f01e46f 100644
--- a/engine/dlib/src/remotery/Remotery.c
+++ b/engine/dlib/src/remotery/Remotery.c
@@ -9766,9 +9766,17 @@ static void RegisterProperty(rmtProperty* property, rmtBool can_lock)
             }
 
             // Calculate the name hash and send it to the viewer
-            name_len = strnlen_s(property->name, 256);
-            property->nameHash = MurmurHash3_x86_32(property->name, name_len, 0);
-            QueueAddToStringTable(g_Remotery->mq_to_rmt_thread, property->nameHash, property->name, name_len, NULL);
+/// DEFOLD
+            const char* name = property->name;
+            if (name[0]=='r' && name[1]=='m' && name[2]=='t' && name[3]=='p' && name[4]=='_')
+            {
+                name = name + 5;
+            }
+
+            name_len = strnlen_s(name, 256);
+            property->nameHash = MurmurHash3_x86_32(name, name_len, 0);
+            QueueAddToStringTable(g_Remotery->mq_to_rmt_thread, property->nameHash, name, name_len, NULL);
+/// END DEFOLD
 
             // Generate a unique ID for this property in the tree
             property->uniqueID = parent_property->uniqueID;

It is also good to consider the fact that since all properties are declared at the same "level" at global scope, you need to keep all property names unique.
E.g a property "Count" would be a common name for a property, so you need to prefix it in some fashion.
That however , doesn't necessarily present well in the gui, so I'll probably remove the name of the parent propery as well:
Example scenario:

rmt_PropertyDefine_Group(rmtp_Particles, "Particles");
rmt_PropertySet_U32(rmtp_ParticlesCount 0, FrameReset, "# particles alive", &rmtp_Particles);

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

No branches or pull requests

2 participants