Questions about the restructuring

[axelson]

In investigating a fix for https://github.com/ScenicFramework/scenic_driver_local/issues/41 it looks like there's multiple issues that have cropped up after https://github.com/ScenicFramework/scenic_driver_local/pull/27

See

• https://github.com/ScenicFramework/scenic_driver_local/issues/47
• https://github.com/ScenicFramework/scenic_driver_local/issues/45
• https://github.com/ScenicFramework/scenic_driver_local/pull/46

Admittedly my c is very rusty, so I may be a bit off base here.

Upon reviewing the code in https://github.com/ScenicFramework/scenic_driver_local/pull/27 more thoroughly I have a few questions:

• Why rename p_ctx to v_ctx?
  • It feels like it was an unnecessary refactoring that ended up breaking bcm and drm modes and it doesn't appear to have driven by a naming conflict
• Why is v_ctx in driver_data_t a void* instead of NVGcontext*?
  • In general I think we should strive to avoid void* and use more specific types instead unless we have a strong reason (and maybe this is one here)
  • Maybe the change to void* could have been done as part of https://github.com/ScenicFramework/scenic_driver_local/pull/31/files instead?

[crertel]

At least with regards to void*, that's the usual way (IIRC) of hiding implementation details. NVGcontext assumes that you're using NanoVG, which isn't always that case, depending. I'd bet that's why it was done.

[ringlej]

Yes, that is the reason for the void*. The cairo work doesn't used NVGcontext. The rename from p_ctx to v_ctx was for 2 reasons:

1. The v was to indicate that this is a void* and the implementation needs to cast it to the proper type of pointer
2. The rename helps with the compiler telling me where this needed to be handled (as it did for you as well with the build breaking for bcm, etc...). It was then intended that at the top of a function where the casting is done, that the concreted pointer variable name would be called p_ctx so that the rest of the function could be left as is.

For example the intended usage pattern for casting would be something like this:

void device_begin_render(driver_data_t* p_data)
{
  NVGcontext* p_ctx = (NVGcontext*)p_data->v_ctx;

  glClear(GL_COLOR_BUFFER_BIT);

  nvgBeginFrame(p_ctx, g_device_info.width, g_device_info.height, g_device_info.ratio);

  // set the global transform
  nvgTransform(p_ctx,
               p_data->global_tx[0], p_data->global_tx[1],
               p_data->global_tx[2], p_data->global_tx[3],
               p_data->global_tx[4], p_data->global_tx[5]);
}

[axelson]

Thanks for the info, that all makes sense. I still think the rename from p_ctx to v_ctx could have been done as a followup to the initial PR, but splitting it out that way doesn't do anything functionally, it just makes the refactor easier to write/review. Closing this since my questions have been answered.