If memory serves the prepareGeometryChange call is left over from a debug effort of when PColorMesh item was first being introduced but there were visible artifacts left over when the mesh changed in some way. It may indeed not be needed.

If memory serves the prepareGeometryChange call is left over from a debug effort of when PColorMesh item was first being introduced but there were visible artifacts left over when the mesh changed in some way. It may indeed not be needed.

I believe both calls are needed, but both should either be called together, or both not called.

It seems rather unsatisfactory to me that there should be an enableAutoLevels state and yet the user is allowed to disable it on a case-by-case basis in a call to setData().

Furthermore, one can only override an enabled state to disabled, but not the other way round.

It seems rather unsatisfactory to me that there should be an enableAutoLevels state and yet the user is allowed to disable it on a case-by-case basis in a call to setData().

Furthermore, one can only override an enabled state to disabled, but not the other way round.

I agree. That doesn't sound ideal.

Hello @SimenZhor, I am doing some refactoring of PCMI in #2879 (merged) and this PR, which would possibly modify the changes introduced in #2463.

What is the use case for having both an overall enableAutoLevels state and a per-data-update autoLevels setting?
The PCMI example exercises only the overall enableAutoLevels state setting.

I would propose to remove the per-data-update autoLevels setting.

Hi @pijyoi , thanks for taking care of the PColorMeshItem!
I don't remember the exact reason behind keeping both enableAutoLevels and autoLevels. But reading back the comments in my PR it looks like I somewhat reluctantly implemented autoLevels to keep the API consistent with ImageItem after feedback provided in code review. See this comment for more info: #2463 (comment)

I don't have an issue with changing this behavior.

If the per-update autoLevels was a requested addition, then instead of removing it, I would propose to change its behavior to be tri-state.
If:

1. not set (i.e. equal to None), then defer to the overall enableAutoLevels state
2. set (to either True / False), then autoLevels over-rides enableAutoLevels.

It seems to me like we all agree that this part of the API is not intuitive or easy to understand. Since it was not clear to you why this solution was implemented, and since this confusion arrived so quickly after my PR, I'm inclined to take that as a sign that the API needs an overhaul (both for PColorMeshItem and ImageItem). Maybe @NilsNemitz wants to weigh in here, since he requested that I implement autoLevels in the first place?

If a breaking API change is not desired, I think your tri-state alternative sounds reasonable.

I certainly won't oppose a nicely thought out improvement to the API.

The autoLevels parameter exists for compatibility with the setImage method of ImageItem, which is functionally very similar.

But having to call that at every update is inconvenient, so enableAutoLevels serves to set a persistent state.

If given, autoLevels should (for this call only) override the persistent state. If not given, the persistent state should be used. I would consider it a bug that it doesn't work like that right now - the "tristate" logic looks more correct. I think we simply didn't work that out properly in the previous PR. (And I didn't check later on, my apologies!)

I think we should add the same enableAutoLevels logic to ImageItem, too. This sets one additional constraint:
If we make enableAutoLevels default to True, then we'll have

• a consistent data setting API across ImageItem and PColorMeshItem
• a convenient persistent setting
• backwards compatibility in ImageItem

The naming of enableAutoLevels parallels enableAutoRange inViewbox, which I believe has a similar function and functionality for axis scaling.
https://pyqtgraph.readthedocs.io/en/latest/api_reference/graphicsItems/viewbox.html#pyqtgraph.ViewBox.enableAutoRange

I am not saying it cannot be simplified, but that was the reasoning in the previous discusion.

changes to ImageItem have been moved to #2883

I know enableAutoLevels and disableAutoLevels pre-date this PR, but looking at this PR I'm wondering why we just didn't do it like any other setter/getter attribute with setAutoLeveling() but that's outside the scope of this PR 👍🏻

Thanks @pijyoi this LGTM, merging.