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.
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.
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.
You can find many good examples for this in wled00/FX.cpp
There was a problem hiding this comment.
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.
Please don't use static variables in an effect function (see previous comment).
There was a problem hiding this comment.
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.
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.
| { | ||
| if (millis() - tetrisai_data->lastTime > msDelayGameOver) | ||
| if (millis() - lastTime > msDelayGameOver) |
There was a problem hiding this comment.
Please use strip.now instead of millis()
There was a problem hiding this comment.
OK it should be very easy to change that.
There was a problem hiding this comment.
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.
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? |
blazoncek
added
effect
waiting for feedback
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. |
With this usermod TetrisAI compiles again.