-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
reset usermod TetrisAI back to initial version #3897
Conversation
fixes #3888 |
As you are using |
So this basicially reverts some of the last commits of the usermod (please correct me if I misunderstood)? Wouldn't it be smarter to resolve the compiler errors in the code, instead of stripping out things until it compiles? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I cannot approve this PR as it is now.
- please avoid using static or global variables in your effect function.
- please use strip.now in your effect function
- I might have misunderstood - but reverting commits "until it compiles" is usually not a good solution. I would suggest to look at the compiler errors in detail, understand the root cause in the code, and then improve the source code to make the compiler happy.
@@ -96,6 +94,8 @@ void drawGrid(TetrisAIGame* tetris, TetrisAI_data* tetrisai_data) | |||
//////////////////////////// | |||
uint16_t mode_2DTetrisAI() | |||
{ | |||
static unsigned long lastTime = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use static variables in an effect function.
You can either use SEGENV.aux0
, SEGENV.aux1
, SEGENV.step
, or allocate persistent data by using SEGENV.allocateData(dataSize)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find many good examples for this in wled00/FX.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to create a 'non blocking' version of the AI part. The commits I took back were related to that.
I need to have a look in the 'static' part.
@@ -116,14 +116,16 @@ uint16_t mode_2DTetrisAI() | |||
//range 0 - 16 | |||
tetrisai_data->colorInc = SEGMENT.custom2 >> 4; | |||
|
|||
if (!tetrisai_data->tetris || (tetrisai_data->tetris.nLookAhead != nLookAhead | |||
static TetrisAIGame tetris(cols < 32 ? cols : 32, rows, 1, piecesData, numPieces); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use static variables in an effect function (see previous comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need a permanent and global object, instantiate it in your usermod class and then use a pointer to it within your effect function.
I would recommend against it though, as user may create multiple segments and have them display the same effect. This should not be prohibited.
You ca also keep the TetrisAI_data
class unchanged but modify TetrisAIGame
class to have a begin()
like method which would take parameters as specified in this constructor. You would use begin(cols < 32 ? cols : 32, rows, 1, piecesData, numPieces);
within if (SEGENV.call == 0) { ... }
condition.
Be mindful though, that segment dimensions may change at any time and cols
may not be the same on 100th call of the effect function than it was at 1st.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I need to keep the 'internal game grid' between calls. The 'effect' is a real game and has an internal state. How would I make sure
- that I provide separate 'game' for every segment (no single static game object possible, I guess)
- the segment is not changed to often as every change will reset and reinitialize the game as the game grid needs to change its shape
I am happy to get this feedback, BTW but struggle a bit to provide solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to agree with @softhack007 and his review comments.
I also tried to provide a hint for better solution than this (which IMO is unacceptable). When you'll be updating the code, please also try to avoid double
variables as they are slow compared to float
counterparts (ESP32 has HW floating point but only for single precision, not double).
@@ -116,14 +116,16 @@ uint16_t mode_2DTetrisAI() | |||
//range 0 - 16 | |||
tetrisai_data->colorInc = SEGMENT.custom2 >> 4; | |||
|
|||
if (!tetrisai_data->tetris || (tetrisai_data->tetris.nLookAhead != nLookAhead | |||
static TetrisAIGame tetris(cols < 32 ? cols : 32, rows, 1, piecesData, numPieces); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need a permanent and global object, instantiate it in your usermod class and then use a pointer to it within your effect function.
I would recommend against it though, as user may create multiple segments and have them display the same effect. This should not be prohibited.
You ca also keep the TetrisAI_data
class unchanged but modify TetrisAIGame
class to have a begin()
like method which would take parameters as specified in this constructor. You would use begin(cols < 32 ? cols : 32, rows, 1, piecesData, numPieces);
within if (SEGENV.call == 0) { ... }
condition.
Be mindful though, that segment dimensions may change at any time and cols
may not be the same on 100th call of the effect function than it was at 1st.
OK I need to test if float is good enough for the AI part. |
@muebau is there any interest in bringing this usermod up to speed and adjust code as requested or do we pull the plug and remove it from WLED? |
Well I will try to find time to fix at least the easy parts ( millis() ). I am still not sure how to target the problem with the state: I need an instance of I would appreciate suggestions from developers with more insight into WLED and C++. |
Do you really need to destroy it? It is just the two variables that change X and Y. Some other effects tackle this so take a look at other 2D effects. |
use of strip.now instead of millis() use float instead of double respond to changes in segment size make effect usable with multible segments
I think this should fix the issues. |
Love to see it is working as it does here. 😂 |
BTW is this a WS2812 4X16X16 setup or something else? |
Yes exactly this, with an added 3d printed grid and diffuser foil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from a few missing f
for float constants this is ok for me.
@softhack007 do you have any objections?
add limitaion information to readme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer you stick to this repository for completeness.
With this usermod TetrisAI compiles again.