ColorSensor: Move rgb() from helper.py to core.py#371
ColorSensor: Move rgb() from helper.py to core.py#371dwalton76 merged 6 commits intoev3dev:developfrom dwalton76:ev3g-api-color-sensor
Conversation
|
For issue #355 |
| Same as raw() but RGB values are scaled to 0-255 | ||
| """ | ||
| (red, green, blue) = self.raw | ||
|
|
There was a problem hiding this comment.
Why integers? It seems like we should return floats given that the sensor theoretically returns 4x the precision as can be represented in this range.
(yes, this is old code, but I'd like to take the chance to make improvements)
There was a problem hiding this comment.
Displaying RGB values in html or passing them to a lot of color libraries need them to be integers.
There was a problem hiding this comment.
If the sensor's accuracy is low enough that 1/4th the resolution isn't a detriment, I have no problem with this; otherwise, I think we either need to heavily document the behavior or add a new accessor/method.
There was a problem hiding this comment.
The accuracy of mine is pretty horrible, I think we are ok dropping the 1/4th resolution
There was a problem hiding this comment.
That certainly sounds familiar 😁 These LEGO sensors are never very accurate.
There was a problem hiding this comment.
It would probably be useful to normalize the values here as well. For example, the blue is always significantly less than red and green. Also, you will never get anywhere near 1020, so the scaled values currently will never be anywhere near 255.
Related discussion: https://www.facebook.com/groups/legomindstorms/permalink/662009207280642/
There was a problem hiding this comment.
Brainstorming....we could add a calibrate_to_white() method where we note the RGB values and then rgb() could scale from 0-255 based on the RGB values we stored when calibrate_to_white() ran.
There was a problem hiding this comment.
That video is interesting, I don't get colors anywhere close to that bright. Let me flip back to the stock ev3 software and try calibrating my sensor.
|
|
||
| return (min(int((red * 255) / self.red_max), 255), | ||
| min(int((green * 255) / self.green_max), 255), | ||
| min(int((blue * 255) / self.blue_max), 255)) |
There was a problem hiding this comment.
Scaling to red_max, green_max, blue_max instead of 1020 makes a HUGE difference in the accuracy of the RGB values returned. I scanned my rubiks cube and displayed the colors returned from rgb() in a web page and they looked pretty good. Before they all looked super super dark.
|
|
||
| @property | ||
| def raw(self): | ||
| """ |
There was a problem hiding this comment.
Should we add a comment here pointing to the calibrate_white method?
There was a problem hiding this comment.
raw() doesn't do anything with it though...it only impacts rgb()
There was a problem hiding this comment.
Something like:
The colors returned by this method may be unexpectedly dark. If this is an issue, check out the rgb() and calibrate_white() methods.
There was a problem hiding this comment.
They scale from 0-1020 though so if you displayed them as if they were 0-255 they would appear fairly bright (assuming none went over the 255 limit). What I'm trying to say is whether they appear dark or not is really up to how the user is using the returned values. If the user is scaling them much like what we do in rgb() then they wouldn't appear that dark.
There was a problem hiding this comment.
My issue is that the doc comment says the range is "0-1020" yet the values will never get up that high. I think that expectation should be laid out in the comment here and in the rgb method.
There was a problem hiding this comment.
ah I follow you now, adding this
+ Red, green, and blue components of the detected color, officially in the
+ range 0-1020 but the values returned will never be that high. We do not
+ yet know why the values returned are low, but pointing the color sensor
+ at a well lit sheet of white paper will return values in the 250-400 range.
+
+ If this is an issue, check out the rgb() and calibrate_white() methods.
There was a problem hiding this comment.
Perfect, I'm happy with this PR.
|
|
||
| return self.value(0) | ||
|
|
||
| @property |
dwalton76
changed the title
3 participants
No description provided.