Skip to content

Regression in 4.4.3: get() and set() are reading and writing from/to the wrong coordinates. #1131

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

Open
PhilipSharman opened this issue Jun 1, 2025 · 5 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@PhilipSharman
Copy link

PhilipSharman commented Jun 1, 2025

Most appropriate sub-area of Processing 4?

Core/Environment/Rendering

Processing version

4.4.3

Operating system

MacOS 11.7.10

Steps to reproduce this

  1. Run the attached script to demonstrate the behavior.

In v4.4.2, the color that get() reads is white, as expected. And set() draws around (350, 350) as expected. But in v4.4.3 (and v4.4.4) get() reads blue, and set() draws around (175, 175).

It appears that both get() and set() are operating on (x/2. y/2) instead of (x,y).

snippet

////////////////////////////////////////////////////////////////////////////////////////////////////////////// //<>// //<>// //<>// //<>//
// Define colors
color white = color(255);
color black = color(0);
color red = color(255, 0, 0);
color blue = color(0, 0, 255);

//////////////////////////////////////////////////////////////////////////////////////////////////////////////
// Setup
void setup() {
    size(400, 400);
    background(white);
    noLoop();
}

//////////////////////////////////////////////////////////////////////////////////////////////////////////////
// Draw
void draw() {
    stroke(blue);
    fill(blue);
    rect(165, 165, 20, 20);

    // BUG 1: The color c is blue but it should be white.
    // It seems to be reading from (x/2, y/2) = (175, 175) instead of (350, 350).
    color c = get(350, 350);
    showColor(c);            // R/G/B = 0.0 0.0 255.0 = blue

    // BUG 2: This draws around (175, 175). It should draw around (350, 350).
    for (int x = 340; x < 360; x++) {
        for (int y = 340; y < 360; y++) {
            set(x, y, red);
        }
    }

    //save("screenshot.xxx.png");
}

//////////////////////////////////////////////////////////////////////////////////////////////////////////////
// Show information about a color
void showColor(color c) {
    print("R/G/B =", red(c), green(c), blue(c) );
    if (c == white) {
        println(" = white");
    } else if (c == blue) {
        println(" = blue");
    } else {
        println("");
    }
}
//////////////////////////////////////////////////////////////////////////////////////////////////////////////

Additional context

No response

Would you like to work on the issue?

No

@PhilipSharman PhilipSharman added the bug Something isn't working label Jun 1, 2025
@PhilipSharman PhilipSharman changed the title Regression in 4.4.3: get() and set() are reading and writing to/from the wrong coordinates. Regression in 4.4.3: get() and set() are reading and writing from/to the wrong coordinates. Jun 1, 2025
@SableRaf
Copy link
Collaborator

SableRaf commented Jun 2, 2025

Hi @PhilipSharman,

This is related to a recent change in Processing 4.4.3 where we updated the default pixelDensity() setting to match your display’s density. It makes sketches look sharper on high-DPI screens, but it has caused a few side effects that we are addressing as they are reported.

Thanks for catching that one!

@SableRaf SableRaf added help wanted Extra attention is needed good first issue Good for newcomers labels Jun 2, 2025
@tychedelia
Copy link

tychedelia commented Jun 2, 2025

I'm not sure that this is a bug -- or at least, the switch to setting pixelDensity by default was a breaking change. The difficulty here is that, as far as I can tell, get/set are currently expressed in terms of physical pixels loaded from the underlying graphics buffer. For sketches prior to forcing a high dpi pixel density in 4.4.3, logical = physical and so that was no problem.

The problem is, if you assume get/set are logical coordinates, this introduces a number of other problems, particularly if fractional scaling support were to be implemented (which it should, I'm guessing things might be wonky on fractional displays atm). Basically, when converting a logical to physical coordinate, what should happen? Should we use nearest neighbor or area sampling? What if the user wants to be able to fetch each physical pixel (i.e. logical sub-pixel)? This could represent a breaking change in the other direction.

I think some solutions here could be to:

Either way @PhilipSharman you should be able to work around this at the moment by setting pixelDensity(1); in your project setup.

@SableRaf
Copy link
Collaborator

SableRaf commented Jun 3, 2025

Hi @tychedelia and thanks for your input and thoughtful suggestions.

I think matching displayDensity was the right call. It makes all sketches look better on high DPI screens by default, and I’d rather not revert it if we can avoid it. That said, we’ll probably need to look more closely at what else this change may have affected.

Besides, I believe most users expect “pixels” to refer to logical coordinates (size() works that way, for example) and probably don't know about pixel density. Introducing the notion of physical pixels too early could be confusing for beginners.

I’d suggest keeping get() and set() in logical space, averaging over physical pixels when needed. That keeps most existing code working as before and matches user expectation.

If necessary for advanced use, a pixelMode(PHYSICAL) flag feels most consistent with Processing’s API design. It would be a breaking change, but only for users already working with pixelDensity(x) and get()/set(), who likely understand the distinction between physical and logical pixels.

@tychedelia
Copy link

tychedelia commented Jun 3, 2025

I think matching displayDensity was the right call. It makes all sketches look better on high DPI screens by default, and I’d rather not revert it unless absolutely necessary.

For sure, it's def the right call from a default perspective. I agree that the logical/physical distinction can be pretty confusing to new users. It hurts my head constantly too! Especially when fractional scaling is involved.

My concern is with the pixels array that the get/set methods point users towards using. Even if you do sampling/blending on the CPU in get/set, the backing buffer will still be misized.

If you're okay with declaring all pixel operations to always be logical, I think the right call then would be to add a downres framebuffer and perform sampling on the GPU, always reading back into a pixel buffer that is the same as size (and vice versa for upres). That way, pixelDensity would only affect the size of allocated GPU textures and not anything on the CPU.

@SableRaf
Copy link
Collaborator

SableRaf commented Jun 3, 2025

That seems reasonable. I'm mostly focused on how it works in user space and the low-level details are a bit over my head ✈️ As long as we agree on the user-facing behavior, I’ll defer to those with a deeper understanding of the optimal implementation. Thanks for taking the time to dig into this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants