Skip to content
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.

Sign up for GitHub

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

Jump to bottom

JavaCPP dependency fixup #605

Open
wants to merge 9 commits into
base: remove-javacv
Choose a base branch
from
Open

JavaCPP dependency fixup #605

wants to merge 9 commits into from

Conversation

ds58
Copy link
Contributor

@ds58 ds58 commented Jan 16, 2025 •
edited
Loading

Depends on #602

ds58
ds58 commented Jan 16, 2025
View reviewed changes
ihmc-graphics/build.gradle.kts Outdated Show resolved Hide resolved
ds58
ds58 commented Jan 16, 2025
View reviewed changes
ihmc-graphics/src/libgdx/java/us/ihmc/rdx/tools/assimp/AssimpOpenedFile.java
@@ -36,7 +42,7 @@ public AssimpOpenedFile(long assimpFileIOAddress, long fileNameAddress, long ope
throw new RuntimeException(message.get());
}

byte[] byteArray = ExceptionTools.handle(() -> IOUtils.toByteArray(url), DefaultExceptionHandler.MESSAGE_AND_STACKTRACE);
byte[] byteArray = ExceptionTools.handle(() -> Files.readAllBytes(filePath), DefaultExceptionHandler.MESSAGE_AND_STACKTRACE);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, IOUtils changed after the dependencies changed. I don't think we should have been using that anyway, since we can just use the java nio Files API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it changed because we have multiple apache commons on the classpath and the load order changed or something, idk

ds58
ds58 commented Jan 16, 2025
View reviewed changes
ihmc-high-level-behaviors/build.gradle.kts
@@ -17,7 +17,9 @@ mainDependencies {
exclude(group = "org.jmonkeyengine")
exclude(group = "org.lwjgl.lwjgl") // exclude lwjgl 2
}
api("us.ihmc:promp-java:1.0.0")
api("us.ihmc:promp-java:1.0.0") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

promp-java uses org.bytedeco:javacpp, which we want to exclude in favor of us.ihmc:javacpp

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably want to re-release it, the exclude stuff doesn't make it into the pom

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thats a good point

ds58
ds58 commented Jan 16, 2025
View reviewed changes
ihmc-perception/src/main/java/us/ihmc/perception/demo/NVCompDemo.java
@@ -73,7 +73,7 @@ public class NVCompDemo

// Bitcomp
protected final BitcompManager bitcompManager;
protected final nvcompBatchedBitcompFormatOpts bitcompOptions;
protected final nvcompBatchedBitcompOpts_t bitcompOptions;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvcomp 4.x change

ds58
ds58 commented Jan 16, 2025
View reviewed changes
ihmc-perception/src/test/java/us/ihmc/perception/cuda/CUDADepthColorizerTest.java
@@ -52,11 +54,11 @@ private Mat readDepthImage()
{
try
{
URL imageURL = RawImageTest.class.getResource("zedDepth16U.raw");
byte[] imageBytes = IOUtils.toByteArray(Objects.requireNonNull(imageURL));
Path path = Paths.get(Objects.requireNonNull(RawImageTest.class.getResource("zedDepth16U.raw")).toURI());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, IOUtils changed. We should be using java nio Files anyway

ds58
ds58 commented Jan 16, 2025
View reviewed changes
ihmc-perception/src/test/java/us/ihmc/perception/cuda/CUDAJPEGProcessorTest.java
@@ -372,11 +374,11 @@ private Mat readBGRImage()
{
try
{
URL imageURL = RawImageTest.class.getResource("zedColorBGR.raw");
byte[] imageBytes = IOUtils.toByteArray(Objects.requireNonNull(imageURL));
Path path = Paths.get(Objects.requireNonNull(RawImageTest.class.getResource("zedColorBGR.raw")).toURI());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, IOUtils changed. We should be using java nio Files anyway

ds58
ds58 commented Jan 16, 2025
View reviewed changes
robot-environment-awareness/build.gradle.kts
Comment on lines -34 to -41
val openblasVersion = "0.3.23-1.5.9"
api("org.bytedeco:openblas:$openblasVersion")
api("org.bytedeco:openblas:$openblasVersion:linux-x86_64")
api("org.bytedeco:openblas:$openblasVersion:windows-x86_64")
val opencvVersion = "4.7.0-1.5.9"
api("org.bytedeco:opencv:$opencvVersion")
api("org.bytedeco:opencv:$opencvVersion:linux-x86_64")
api("org.bytedeco:opencv:$opencvVersion:windows-x86_64")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These come from the logger

ds58
ds58 commented Jan 16, 2025
View reviewed changes
ihmc-perception/build.gradle.kts
Comment on lines -30 to -45
api("org.bytedeco:openblas:$openblasVersion")
api("org.bytedeco:openblas:$openblasVersion:linux-x86_64")
api("org.bytedeco:openblas:$openblasVersion:linux-arm64")
api("org.bytedeco:openblas:$openblasVersion:windows-x86_64")
val opencvVersion = "4.7.0-1.5.9"
api("org.bytedeco:opencv:$opencvVersion")
api("org.bytedeco:opencv:$opencvVersion:linux-x86_64")
api("org.bytedeco:opencv:$opencvVersion:linux-arm64")
api("org.bytedeco:opencv:$opencvVersion:windows-x86_64")
api("org.bytedeco:opencv:$opencvVersion:linux-x86_64-gpu")
api("org.bytedeco:opencv:$opencvVersion:windows-x86_64-gpu")
val ffmpegVersion = "6.0-1.5.9"
api("org.bytedeco:ffmpeg:$ffmpegVersion")
api("org.bytedeco:ffmpeg:$ffmpegVersion:linux-x86_64")
api("org.bytedeco:ffmpeg:$ffmpegVersion:linux-arm64")
api("org.bytedeco:ffmpeg:$ffmpegVersion:windows-x86_64")
Copy link
Contributor Author

@ds58 ds58 Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openblas, opencv, ffmpeg come from the logger https://github.com/ihmcrobotics/ihmc-robot-data-logger/pull/39/files

ds58
ds58 commented Jan 16, 2025
View reviewed changes
ihmc-perception/src/main/java/us/ihmc/perception/cuda/CUDACompressionTools.java
@@ -64,9 +64,9 @@ public CUDACompressionTools()

public void destroy()
{
compressionManager.close();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to close compressionManager before releasing the stream (fix by @TomaszTB )

ds58
ds58 commented Jan 16, 2025
View reviewed changes
ihmc-perception/src/test/java/us/ihmc/perception/cuda/CUDACompressionToolsTest.java
@@ -91,13 +91,13 @@ public void testGPUDepthCompressionDecompression() throws IOException
@Test
public void testBasicCompression() throws IOException
{
CUDACompressionTools compressor = new CUDACompressionTools();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this to load cudart, which loads the jni for Pointer, BytePointer. We were trying to use BytePointer first, which didn't work when this test was run alone.

@ds58 ds58 marked this pull request as ready for review January 16, 2025 22:54
TomaszTB
TomaszTB reviewed Jan 16, 2025
View reviewed changes
ihmc-perception/src/main/java/us/ihmc/perception/cuda/CUDACompressionTools.java
@@ -56,17 +56,16 @@ public CUDACompressionTools()
stream = CUDAStreamManager.getStream();

nvcompBatchedZstdOpts_t zstdOptions = nvcomp.nvcompBatchedZstdDefaultOpts();
compressionManager = new ZstdManager(CHUNK_SIZE, zstdOptions, stream, 0, nvcomp.NoComputeNoVerify);
compressionManager = new ZstdManager(CHUNK_SIZE, zstdOptions, stream, nvcomp.NoComputeNoVerify, 0 /*NVCOMP_NATIVE*/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructor parameters changed from nvcomp 3.x to 4.x

TomaszTB
TomaszTB reviewed Jan 16, 2025
View reviewed changes
ihmc-perception/src/main/java/us/ihmc/perception/cuda/CUDACompressionTools.java
@@ -387,7 +387,7 @@ public static BytePointer decompress(BytePointer compressedData, long compressed
checkCUDAError(cudaMemcpyAsync(compressedDeviceBuffer, compressedData, compressedDataSize, cudart.cudaMemcpyDefault, cudaStream));

// Create a decompression manager
nvcompManagerBase decompressionManager = nvcomp.create_manager(compressedDeviceBuffer, cudaStream, 0, nvcomp.NoComputeNoVerify);
nvcompManagerBase decompressionManager = nvcomp.create_manager(compressedDeviceBuffer, cudaStream, nvcomp.NoComputeNoVerify);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameters changed from nvcomp 3.x to 4.x

calvertdw
calvertdw reviewed Jan 16, 2025
View reviewed changes
ihmc-avatar-interfaces/build.gradle.kts
@@ -16,7 +16,9 @@ mainDependencies {
api("com.hierynomus:sshj:0.32.0")

api("us.ihmc:mecano-graphviz:17-0.18.1")
api("us.ihmc:scs2-bullet-simulation:17-0.28.3")
api("us.ihmc:scs2-bullet-simulation:17-0.28.3") {
exclude("org.bytedeco", "javacv")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this exclude?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scs2 currently includes javacv. It won't in a later version, but we still have to release that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this works transitively, so there may still be issues

calvertdw
calvertdw reviewed Jan 16, 2025
View reviewed changes
ihmc-perception/build.gradle.kts
}
// ffmpeg, openblas, opencv come from the logger

val cudaVersion = "12.6-9.5-1.5.11-ihmc-2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could extract "1.5.11-ihmc-2" to a variable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see the point, it would just make it more confusing to look at imo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with me

TomaszTB
TomaszTB reviewed Jan 21, 2025
View reviewed changes
ihmc-perception/README.md
tar -xvf nvcomp-linux-x86_64-4.1.0.6_cuda12-archive.tar.xz
sudo rsync -av nvcomp-linux-x86_64-4.1.0.6_cuda12-archive/lib/* /usr/local/cuda/lib64
sudo rsync -av nvcomp-linux-x86_64-4.1.0.6_cuda12-archive/include/* /usr/local/cuda/include
rm -rf nvcomp-linux-x86_64-4.1.0.6_cuda12-archive*
```

### ZED SDK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update this 4.2.1 -> 4.2.3 while we're updating this file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomaszTB TomaszTB TomaszTB left review comments

@calvertdw calvertdw calvertdw left review comments

@PotatoPeeler3000 PotatoPeeler3000 Awaiting requested review from PotatoPeeler3000

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ds58 @calvertdw @TomaszTB